[WIP][WOC] feedback implementation (continuation of #1464) #3424

kuhnchris · 2019-01-05T14:35:08Z

Description

User-Feedback implementation

Checklist

Read and conforms to the contributor guidelines
Read and conforms to the style guidelines
Linter status: 100% pass
Added relevant tests
Tests pass and code coverage has not decreased
Changes don't break existing behaviour
Commit message follows commit guidelines
Code documentation
Completed: Code review by core team and any requested changes

Affected core subsystem(s)

ui, db, backend, frontend, tips, bounty

Refers/Fixes

Refs: #1464

Testing and Sign-off

/

Contributor

Read and followed the Contributor Guidelines
Tested all changes locally
Verified existing functionality
Ran make test and everything passed!

Reviewer

Affirm contributor guidelines have been followed and requested changes made
CI tests and linting pass
No conflicts (migrations, files, etc)
Regression tested against staging

Funder

Validated requested changes were made to specification
Bounty payout released to the contributor

stickler-ci · 2019-01-05T14:35:33Z

app/assets/v2/js/pages/fulfill_bounty.js

@@ -45,7 +59,7 @@ window.onload = function() {
        var notificationEmail = data.notificationEmail;
        var githubPRLink = data.githubPRLink;
        var hoursWorked = data.hoursWorked;
-
+		debugger;


Expected indentation of 8 spaces but found 2 tabs. (indent)
Unexpected 'debugger' statement. (no-debugger)

stickler-ci · 2019-01-05T14:35:33Z

app/assets/v2/js/pages/fulfill_bounty.js

-          }
+          },
+		  review: {
+			  rating: data.rating?data.rating:-1,


Infix operators must be spaced. (space-infix-ops)

stickler-ci · 2019-01-05T14:35:33Z

app/assets/v2/js/pages/fulfill_bounty.js

+          },
+		  review: {
+			  rating: data.rating?data.rating:-1,
+			  comment: data.review?data.review:"No comment given.",


Infix operators must be spaced. (space-infix-ops)
Strings must use singlequote. (quotes)

stickler-ci · 2019-01-05T14:35:33Z

app/assets/v2/js/pages/fulfill_bounty.js

+		  review: {
+			  rating: data.rating?data.rating:-1,
+			  comment: data.review?data.review:"No comment given.",
+			  reviewType: "worker"


Strings must use singlequote. (quotes)

stickler-ci · 2019-01-05T14:35:33Z

app/dashboard/helpers.py

                    user_profile = sync_profile(fulfillment.fulfiller_github_username)
+


W293 blank line contains whitespace

stickler-ci · 2019-01-05T14:35:33Z

app/dashboard/models.py

@@ -999,6 +999,21 @@ def submitted(self):
        """Exclude results that have not been submitted."""
        return self.exclude(fulfiller_address='0x0000000000000000000000000000000000000000')

+class BountyComment(SuperModel):


E302 expected 2 blank lines, found 1

stickler-ci · 2019-01-05T14:35:34Z

app/dashboard/models.py

@@ -999,6 +999,21 @@ def submitted(self):
        """Exclude results that have not been submitted."""
        return self.exclude(fulfiller_address='0x0000000000000000000000000000000000000000')

+class BountyComment(SuperModel):
+    fulfillment = models.ForeignKey(BountyFulfillment, related_name='comments', on_delete=models.CASCADE)


F821 undefined name 'BountyFulfillment'

stickler-ci · 2019-01-05T14:35:34Z

app/dashboard/models.py

+    rating = models.IntegerField(null=True, blank=True)
+    commentsender = models.ForeignKey('dashboard.Profile', related_name='comments_given', on_delete=models.CASCADE)
+    commentreceiver = models.ForeignKey('dashboard.Profile', related_name='comments_received', on_delete=models.CASCADE)
+


W293 blank line contains whitespace

thelostone-mc

Let a few minor comments. Could you fix up the sticker comments and also throw in a working demo ?

thelostone-mc · 2019-01-05T14:48:48Z

app/assets/v2/css/base.css

+.rating {
+  width:160px;
+}
+.rating span { float:right; position:relative; }


Empty line between css rules :P

thelostone-mc · 2019-01-05T14:49:10Z

app/assets/v2/js/pages/fulfill_bounty.js

+  $('.rating input').click(function() {
+    $('.rating span').removeClass('checked');
+    $(this).parent().addClass('checked');
+  });


Arrow functions :P

thelostone-mc · 2019-01-05T14:49:59Z

app/assets/v2/js/pages/fulfill_bounty.js

+  $('input:radio').change(
+    function() {
+      var userRating = this.value;
+    });


Is this variable used anywhere?

kuhnchris · 2019-01-05T15:03:28Z

wait, wait, this is FAR from being finished. This is a WIP and WOC ;-)
I'll ping you if I need a review on this, OK @thelostone-mc ? :-)

owocki · 2019-01-07T17:41:33Z

excited for this :)

kuhnchris · 2019-01-07T19:04:29Z

Same, but ETHDenver has prio 1 ;-)

…eedback

object to helper.py and utils.py.

stickler-ci · 2019-01-11T08:53:26Z

app/dashboard/admin.py

@@ -68,6 +69,9 @@ class UserActionAdmin(admin.ModelAdmin):
    search_fields = ['action', 'ip_address', 'metadata', 'profile__handle']
    ordering = ['-id']

+class FeedbackAdmin(admin.ModelAdmin):


E302 expected 2 blank lines, found 1

stickler-ci · 2019-01-11T08:53:26Z

app/dashboard/admin.py

@@ -68,6 +69,9 @@ class UserActionAdmin(admin.ModelAdmin):
    search_fields = ['action', 'ip_address', 'metadata', 'profile__handle']
    ordering = ['-id']

+class FeedbackAdmin(admin.ModelAdmin):
+    search_fields = ['sender_profile','receiver_profile','bounty','feedbackType']


E231 missing whitespace after ','

stickler-ci · 2019-01-11T08:53:26Z

app/dashboard/helpers.py

                return (did_change, latest_old_bounty, new_bounty)
            return (did_change, latest_old_bounty, latest_old_bounty)
    except SemaphoreExists:
        return (did_change, latest_old_bounty, latest_old_bounty)

+def create_new_feedback(new_bounty, feedback_dict):


E302 expected 2 blank lines, found 1

stickler-ci · 2019-01-11T08:53:26Z

app/dashboard/helpers.py

                return (did_change, latest_old_bounty, new_bounty)
            return (did_change, latest_old_bounty, latest_old_bounty)
    except SemaphoreExists:
        return (did_change, latest_old_bounty, latest_old_bounty)

+def create_new_feedback(new_bounty, feedback_dict):
+    feedbackType = feedback_dict.get('feedbackType','???')


E231 missing whitespace after ','

stickler-ci · 2019-01-11T08:53:27Z

app/dashboard/models.py

@@ -2778,3 +2777,33 @@ class BlockedUser(SuperModel):
    def __str__(self):
        """Return the string representation of a Bounty."""
        return f'<BlockedUser: {self.handle}>'
+
+class FeedbackEntry(SuperModel):


E302 expected 2 blank lines, found 1

stickler-ci · 2019-01-11T08:53:27Z

app/dashboard/utils.py

@@ -361,6 +361,7 @@ def get_bounty(bounty_enum, network):
        'token': token,
        'fulfillments': fulfillments,
        'network': network,
+        'review': bounty_data.get('review',{}),


E231 missing whitespace after ','

SaptakS · 2019-01-15T07:44:31Z

@kuhnchris can you please fix the linting issue so that travis passes?

kuhnchris · 2019-01-15T07:45:42Z

as repeatedly mentioned: this is not finished.
Yes, I will fix linting, but please let me finish the task first and then do the linting, else we'd do that douzens of times.

kziemianek · 2019-01-15T07:57:55Z

app/dashboard/helpers.py

                return (did_change, latest_old_bounty, new_bounty)
            return (did_change, latest_old_bounty, latest_old_bounty)
    except SemaphoreExists:
        return (did_change, latest_old_bounty, latest_old_bounty)

+def create_new_feedback(new_bounty, feedback_dict):


ever considered putting this logic into model ? models looks anemic and you create 'create' method in helpers while it can belong to Feedback itself

@classmethod worker_feedback(cls,bounty,...) return cls() @classmethod other_worker_feedback(cls,bounty,...) return cls()

yes, considered that already. The main thing was to get it working, since we are having a IPFS bridge here and it wasn't working very well. especially because we do not simply provide the IPFS later on to the create functions, but actually parse them and return a DIFFERENT json structure than the one we pass into IPFS. Hence those methods are still from the "trying to get this to work" era.

codecov · 2019-01-20T13:15:52Z

Codecov Report

Merging #3424 into master will decrease coverage by 0.02%.
The diff coverage is 35%.

@@            Coverage Diff             @@
##           master    #3424      +/-   ##
==========================================
- Coverage   29.73%   29.71%   -0.03%     
==========================================
  Files         185      185              
  Lines       14574    14610      +36     
  Branches     1929     1931       +2     
==========================================
+ Hits         4334     4341       +7     
- Misses      10101    10130      +29     
  Partials      139      139

Impacted Files	Coverage Δ
app/dashboard/utils.py	`35.46% <ø> (ø)`	⬆️
...ard/management/commands/create_activity_records.py	`0% <0%> (ø)`	⬆️
app/dashboard/admin.py	`71.05% <100%> (+1.05%)`	⬆️
app/dashboard/helpers.py	`16% <8.69%> (-0.62%)`	⬇️
app/dashboard/models.py	`53.09% <88.88%> (+0.22%)`	⬆️
app/dashboard/embed.py	`28.16% <0%> (-3.45%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2796f57...370237c. Read the comment docs.

codecov · 2019-01-20T13:15:52Z

Codecov Report

Merging #3424 into master will decrease coverage by 0.06%.
The diff coverage is 42.85%.

@@            Coverage Diff            @@
##           master   #3424      +/-   ##
=========================================
- Coverage   29.76%   29.7%   -0.07%     
=========================================
  Files         193     201       +8     
  Lines       15045   15658     +613     
  Branches     1971    2056      +85     
=========================================
+ Hits         4478    4651     +173     
- Misses      10415   10850     +435     
- Partials      152     157       +5

Impacted Files	Coverage Δ
app/app/context.py	`57.5% <ø> (ø)`	⬆️
app/app/urls.py	`90% <100%> (+0.2%)`	⬆️
app/app/settings.py	`78.43% <33.33%> (+0.35%)`	⬆️
app/economy/models.py	`57.14% <0%> (-20.64%)`	⬇️
app/grants/admin.py	`41.97% <0%> (-5.49%)`	⬇️
app/dashboard/helpers.py	`15.77% <0%> (-0.75%)`	⬇️
app/marketing/views.py	`12.26% <0%> (-0.72%)`	⬇️
app/grants/views.py	`12.7% <0%> (-0.68%)`	⬇️
app/retail/utils.py	`11.43% <0%> (-0.54%)`	⬇️
app/kudos/views.py	`22.04% <0%> (-0.35%)`	⬇️
... and 33 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 978e30b...fe013c9. Read the comment docs.

PixelantDesign · 2019-01-23T04:06:50Z

Hi @kuhnchris any updates here? :)

kuhnchris · 2019-01-23T18:34:35Z

Yeah, as we discussed - making a video/recording to show you and finishing it up ;-)

…eedback

danlipert

This is looking good @kuhnchris ! I've got a couple suggestions, as well as some questions (and @thelostone-mc has some unanswered questions too). Lets get those addressed, as well as removing the leftover logging statements and style issues. Would also love to see some tests if you've got time! Thanks!

danlipert · 2019-02-01T16:04:04Z

app/app/urls.py

@@ -109,6 +109,7 @@
    # dashboard views
    re_path(r'^onboard/(?P<flow>\w+)/$', dashboard.views.onboard, name='onboard'),
    re_path(r'^onboard/contributor/avatar/?$', dashboard.views.onboard_avatar, name='onboard_avatar'),
+    url(r'^postComment/', dashboard.views.postComment, name='postComment'),


What do you think about changing the naming here to just comment since the post is implied as we're creating a comment resource?

sure, can do.

danlipert · 2019-02-01T16:07:17Z

app/dashboard/helpers.py

@@ -333,7 +333,9 @@ def handle_bounty_fulfillments(fulfillments, new_bounty, old_bounty):
        except Exception as e:
            logger.error(f'{e} during new fulfillment creation for {new_bounty}')
            continue
-    return new_bounty.fulfillments.all()
+
+    if new_bounty:


What is the goal of this check on new_bounty?

danlipert · 2019-02-01T16:08:59Z

app/dashboard/helpers.py

@@ -624,18 +649,25 @@ def record_bounty_activity(event_name, old_bounty, new_bounty, _fulfillment=None
                          'increased_bounty',
                          'worker_rejected',
                          'bounty_changed']
+        logger.info(f"record_bounty_activity > profile at start: {user_profile}")


Lets remove these extra logging statements before we merge this PR 👍

danlipert · 2019-02-01T16:09:36Z

app/dashboard/helpers.py

@@ -728,6 +760,8 @@ def process_bounty_changes(old_bounty, new_bounty):
    # record a useraction for this
    record_user_action(event_name, old_bounty, new_bounty)
    record_bounty_activity(event_name, old_bounty, new_bounty)
+    # check for review{} in here, create review object, wheeee.


What is this comment referring to?

danlipert · 2019-02-01T16:13:56Z

app/dashboard/models.py

+    )
+    rating = models.SmallIntegerField(blank=True, default=0)
+    comment = models.TextField(default='', blank=True)
+    feedbackType = models.TextField(default='', blank=True, max_length=20)


It looks like there are only two types of feedback: approver and worker. If thats the case, what do you think about using the choices argument here to add a bit of validation and make the admin interface that much nicer? :)

danlipert · 2019-02-01T16:16:59Z

app/dashboard/views.py

@@ -300,6 +301,42 @@ def new_interest(request, bounty_id):
    })


+@require_POST
+def postComment(request):


Lets use lowercase with underscores style here to match with PEP 8: https://www.python.org/dev/peps/pep-0008/#naming-conventions. Also, since this function handles a POST request as evidenced by the decorator, maybe we should just call it comment or maybe new_comment. What do you think?

danlipert · 2019-02-01T16:19:49Z

app/dashboard/views.py

+        'sender_profile': profile_id,
+        'receiver_profile': bountyObj.fulfillments.last().profile,
+        'rating': feedback_dict.get('rating', '-1'),
+        'comment': feedback_dict.get('comment', 'No comment.'),


I worry about potential script injection issues here due to the untrusted data: OWASP Wiki.

However, maybe santizing that is beyond the scope of V1 of this feature. Lets add a note to make sure we don't forget that this data is untrusted and unsanitized 👍

owocki · 2019-02-08T00:28:00Z

hey @kuhnchris how goes? ETHDenver is next week.. so hoping to get this in this upcoming monday's deploy. need anything from us? did u see @danlipert 's code comments?

thelostone-mc · 2019-02-12T19:23:05Z

Closing as we've got the latest at #3709

feedback implementation (continuation of #3424)

owocki and others added 11 commits June 16, 2018 14:19

feedback system WIP

f662ff1

Merge branch 'master' into kevin/feedback_system

1635061

eslint/stylint

e7d9b13

Random formatting

19729c9

Merge branch 'master' into kevin/feedback_system

ca6644c

Merge branch 'master' into kevin/feedback_system

3323668

Merge branch 'master' into kevin/feedback_system

fdcf1de

Merge branch 'master' into kevin/feedback_system

4ccbda7

Merge branch 'master' into kevin/feedback_system

bc7e5b1

Merge remote-tracking branch 'origin/master' into kevin/feedback_system

f39645e

WIP - feedback system continuation

9ed8d67

stickler-ci reviewed Jan 5, 2019

View reviewed changes

thelostone-mc reviewed Jan 5, 2019

View reviewed changes

PixelantDesign added Q1 2019 labels Jan 8, 2019

kuhnchris added 2 commits January 10, 2019 20:05

Merge branch 'master' of https://github.com/gitcoinco/web into 1464-f…

38f1364

…eedback

New Migration for new database object, feedback saving, added 'review'

dfa7783

object to helper.py and utils.py.

stickler-ci reviewed Jan 11, 2019

View reviewed changes

kziemianek reviewed Jan 15, 2019

View reviewed changes

thelostone-mc added the in progress label Jan 15, 2019

PixelantDesign mentioned this pull request Jan 18, 2019

Q1 Green Team Initiatives #3565

Closed

attempt to finish this...

370237c

Added postComment, new PostComment target, feedback

19d6184

kuhnchris and others added 2 commits January 24, 2019 18:20

Another commit...

822c107

Merge branch 'master' of https://github.com/gitcoinco/web into 1464-f…

7ceb038

…eedback

PixelantDesign added the will tip for review label Jan 30, 2019

danlipert suggested changes Feb 1, 2019

View reviewed changes

Merge branch 'master' into 1464-feedback

fe013c9

danlipert mentioned this pull request Feb 8, 2019

feedback implementation (continuation of #3424) #3709

Merged

thelostone-mc closed this Feb 12, 2019

thelostone-mc added a commit that referenced this pull request Mar 7, 2019

Merge pull request #3709 from gitcoinco/pr/3424

ef43896

feedback implementation (continuation of #3424)

		user_profile = sync_profile(fulfillment.fulfiller_github_username)

Uh oh!

[WIP][WOC] feedback implementation (continuation of #1464) #3424

[WIP][WOC] feedback implementation (continuation of #1464) #3424

Uh oh!

Conversation

kuhnchris commented Jan 5, 2019

Description

Checklist

Affected core subsystem(s)

Refers/Fixes

Testing and Sign-off

Contributor

Reviewer

Funder

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thelostone-mc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kuhnchris commented Jan 5, 2019

Uh oh!

owocki commented Jan 7, 2019

Uh oh!

kuhnchris commented Jan 7, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SaptakS commented Jan 15, 2019

Uh oh!

kuhnchris commented Jan 15, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jan 20, 2019

Codecov Report

Uh oh!

codecov bot commented Jan 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

PixelantDesign commented Jan 23, 2019

Uh oh!

kuhnchris commented Jan 23, 2019

Uh oh!

danlipert left a comment

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jan 20, 2019 •

edited

Loading