allow funder to turn off auto approvals at bounty creation #1945

darkdarkdragon · 2018-08-05T23:43:25Z

Description

Allow funder to turn off auto approvals at bounty creation

Checklist

linter status: 100% pass
changes don't break existing behavior

Testing

Tested manually

Refers/Fixes

Fixes #1807

stickler-ci · 2018-08-05T23:43:52Z

app/dashboard/helpers.py

@@ -407,6 +407,7 @@ def create_new_bounty(old_bounties, bounty_payload, bounty_details, bounty_id):
                experience_level=metadata.get('experienceLevel', '') if not latest_old_bounty else latest_old_bounty.experience_level,
                project_type=bounty_payload.get('schemes', {}).get('project_type', 'traditional') if not latest_old_bounty else latest_old_bounty.project_type,
                permission_type=bounty_payload.get('schemes', {}).get('permission_type', 'permissionless') if not latest_old_bounty else latest_old_bounty.permission_type,
+                auto_approve_workers=bounty_payload.get('schemes', {}).get('auto_approve_workers', 1) if not latest_old_bounty else latest_old_bounty.auto_approve_workers,


E501 line too long (171 > 120 characters)

codecov · 2018-08-05T23:47:32Z

Codecov Report

Merging #1945 into master will decrease coverage by <.01%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #1945      +/-   ##
==========================================
- Coverage   27.78%   27.77%   -0.01%     
==========================================
  Files         145      145              
  Lines       11627    11628       +1     
  Branches     1570     1570              
==========================================
  Hits         3230     3230              
- Misses       8285     8286       +1     
  Partials      112      112

Impacted Files	Coverage Δ
app/dashboard/router.py	`28.26% <ø> (ø)`	⬆️
app/dashboard/helpers.py	`16.98% <0%> (-0.06%)`	⬇️

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 aec34fb...4e97e68. Read the comment docs.

PixelantDesign · 2018-08-06T11:54:59Z

@darkdarkdragon are you able to provide screenshots? Thanks!

darkdarkdragon · 2018-08-06T13:11:25Z

@PixelantDesign I've prepared them, but forgot to upload :(
will prepare new ones in couple of hours

darkdarkdragon · 2018-08-06T23:20:00Z

darkdarkdragon · 2018-08-06T23:20:13Z

stickler-ci · 2018-08-06T23:20:14Z

app/dashboard/helpers.py

@@ -407,6 +407,8 @@ def create_new_bounty(old_bounties, bounty_payload, bounty_details, bounty_id):
                experience_level=metadata.get('experienceLevel', '') if not latest_old_bounty else latest_old_bounty.experience_level,
                project_type=bounty_payload.get('schemes', {}).get('project_type', 'traditional') if not latest_old_bounty else latest_old_bounty.project_type,
                permission_type=bounty_payload.get('schemes', {}).get('permission_type', 'permissionless') if not latest_old_bounty else latest_old_bounty.permission_type,
+                auto_approve_workers=bounty_payload.get('schemes', {}).
+                    get('auto_approve_workers', 1) if not latest_old_bounty else latest_old_bounty.auto_approve_workers,


E131 continuation line unaligned for hanging indent

darkdarkdragon · 2018-08-06T23:20:22Z

stickler-ci · 2018-08-06T23:28:24Z

app/dashboard/helpers.py

@@ -407,6 +408,7 @@ def create_new_bounty(old_bounties, bounty_payload, bounty_details, bounty_id):
                experience_level=metadata.get('experienceLevel', '') if not latest_old_bounty else latest_old_bounty.experience_level,
                project_type=bounty_payload.get('schemes', {}).get('project_type', 'traditional') if not latest_old_bounty else latest_old_bounty.project_type,
                permission_type=bounty_payload.get('schemes', {}).get('permission_type', 'permissionless') if not latest_old_bounty else latest_old_bounty.permission_type,
+                auto_approve_workers= aaw if not latest_old_bounty else latest_old_bounty.auto_approve_workers,


E251 unexpected spaces around keyword / parameter equals

thelostone-mc · 2018-08-07T03:21:53Z

Should we display auto approval option only if the logged in user is the funder ?

PixelantDesign · 2018-08-07T03:46:43Z

@darkdragon could we update the checkbox language to

Auto approve worker after 72 hours.

Let's do checked by default so that there aren't a bunch of issues held up from issues not being checked.

For the Issue Details looks good for this iteration.

@thelostone-mc
I think we should display for anyone that wants to fund....I don't feel strongly about that logic.

@owocki @mbeacom @saptak?

darkdarkdragon · 2018-08-07T08:47:42Z

@PixelantDesign fixed checkbox's text
checkbox is already checked by default

PixelantDesign · 2018-08-07T10:52:22Z

Great!

PixelantDesign · 2018-08-09T13:17:13Z

@darkdarkdragon will you look at the above conflicts?

darkdarkdragon · 2018-08-09T14:14:10Z

@PixelantDesign I've resolved conflicts.
But, btw, it is problem - I'm creating pr, it sits there without getting merged -> new commits to master -> my pr getting conflicts -> I'm resolving them -> it sits there -> rinse, repeat till starts are aligned in proper they and my pr finally merged.
It's pretty frustrating experience :(

thelostone-mc · 2018-08-10T19:22:28Z

@darkdarkdragon Ah I know it can get frustrating but usually what happens is -> when a PR is shot out & the triggered build fails -> we can't merge it in just the logic works.
By the time you get to it , we end up merging commits from other contributors as well as core contributors which may end up in conflict resolution.

Sorry about the merge conflicts. It's something which is kinda occasionally impossible to avoid 😅

mbeacom · 2018-08-09T12:23:54Z

app/dashboard/helpers.py

@@ -374,6 +374,7 @@ def create_new_bounty(old_bounties, bounty_payload, bounty_details, bounty_id):
            old_bounty.save()
            latest_old_bounty = old_bounty
        try:
+            aaw = bounty_payload.get('schemes', {}).get('auto_approve_workers', 1)


Can we be more explicit with this variable name?

mbeacom · 2018-08-10T20:00:28Z

app/dashboard/models.py

@@ -260,6 +260,7 @@ class Bounty(SuperModel):
    project_type = models.CharField(max_length=50, choices=PROJECT_TYPES, default='traditional')
    permission_type = models.CharField(max_length=50, choices=PERMISSION_TYPES, default='permissionless')
    snooze_warnings_for_days = models.IntegerField(default=0)
+    auto_approve_workers = models.BooleanField(default=True)


Can't we simply reuse the Bounty.admin_override_suspend_auto_approval field for this? If we're worried about the naming convention, rename and migrate the field?
Then you could simply update the JS to display this button if staff OR funder and display Suspend or Enable based on the current state?

We can. I've added new field because this two have slightly different meaning, and I was not sure I can merge them. Also, we was planning to add check to the bounty details page so funder can turn it off/on at any time, and in that case, if we merge those two fields, admin will not be able to override it in a way so that funder will not be able to flip it back. I was not sure if this is acceptable.

@mbeacom your thoughts on that one?

@darkdarkdragon I don't think we're too concerned with staff not being able to forcibly disable/enable auto approval and ensure persistence here. In an effort to increase maintainability moving forward, maybe we could rename the field, but simply use one field to handle this.

@mbeacom ok, no problem, I'll remove extra field

@frankchen07 either way. If somebody point me to relevant example how to do it, I'll do it, but if someone else can do it faster that's fine too

@darkdarkdragon make migrations will generate the automatic migrations. make migrate migrates your local DB with the new migration changes.

@darkdarkdragon What I was recommending was to delete your new migrations and field and simply use the original field for this. If you are unsure of how to migrate a field to a new field name (not entirely required at the moment), please just delete your migrations in this branch, remove your new field, leave the original field and use that.

Simply switch back to using admin_override_suspend_auto_approval instead of auto_approve_workers for the time being.

ok, will do it tomorrw

frankchen07 · 2018-08-15T23:54:36Z

hey @darkdarkdragon - I'm helping @PixelantDesign do some check-ins on our August bountied milestones. Have you gotten a chance to look at @mbeacom's change requests?

darkdarkdragon · 2018-08-16T08:35:33Z

@frankchen07 oops, somehow missed that change request. Will do it after we settle with mbeacom on this topic

thelostone-mc · 2018-09-06T04:47:22Z

@darkdarkdragon rebase whenever it's ready to be reviewed

stickler-ci · 2018-09-09T23:05:03Z

Your configuration file uses an unknown linter. The eslint linter is not a supported linter.

darkdarkdragon · 2018-09-09T23:12:30Z

@thelostone-mc @mbeacom this ready to be reviewed

thelostone-mc

LGTM. works fine on local 👍
@mbeacom should we test this out on staging too ?

PixelantDesign · 2018-09-24T13:40:47Z

yay! @mbeacom can we see this on staging?

mbeacom · 2018-09-24T13:59:53Z

@PixelantDesign This is on staging.
@darkdarkdragon Thanks for the contribution! Once we get this signed off, we'll merge it! 😄

mbeacom · 2018-09-24T14:02:27Z

SaptakS

Left some comments. Please check.

SaptakS · 2018-09-24T14:15:33Z

app/assets/v2/js/pages/bounty_details.js

@@ -178,6 +179,14 @@ var callbacks = {
  'project_type': function(key, val, result) {
    return [ 'project_type', ucwords(result.project_type) ];
  },
+  'admin_override_suspend_auto_approval': function(key, val, result) {
+    if (result['permission_type'] == 'approval') {


Please use === instead of ==

SaptakS · 2018-09-24T14:17:29Z

app/assets/v2/js/pages/new_bounty.js

+  $('select[name=permission_type]').change(function() {
+    var val = $('select[name=permission_type] option:selected').val();
+
+    if (val == 'approval') {


Please use === instead of ==

SaptakS · 2018-09-24T14:19:02Z

app/assets/v2/js/pages/new_bounty.js

@@ -105,6 +105,15 @@ $(document).ready(function() {
  waitforWeb3(function() {
    promptForAuth();
  });
+  $('select[name=permission_type]').change(function() {


Please use .on('change', function(){}). .change, .click and similar action listeners functions have been deprecated by jQuery.

SaptakS · 2018-09-24T14:21:04Z

app/dashboard/templates/shared/issue_details.html

+<div class="w-100 mt-2" id="auto_approve_workers_container">
+  <div class="form__checkbox">
+    <input name="admin_override_suspend_auto_approval" id="admin_override_suspend_auto_approval" type="checkbox" value="1" checked />
+    <label class="form__label" for="admin_override_suspend_auto_approval" style="display: flex;">{% blocktrans %}Worker will be


Please use {% trans "" %} instead of {% blocktrans %}

darkdarkdragon · 2018-10-02T08:00:32Z

@SaptakS @mbeacom done

pinkiebell

Everything else lgtm 👍

app/dashboard/templates/shared/issue_details.html

PixelantDesign · 2018-10-18T12:32:25Z

Yay! Can we merge?

mbeacom

🤷‍♂️ - we can adjust the remaining comments after it's been merged.

stickler-ci reviewed Aug 5, 2018

View reviewed changes

gitcoinbot mentioned this pull request Aug 6, 2018

BUILD: Allow Funder to Manage Auto Approvals #1807

Closed

5 tasks

thelostone-mc requested review from thelostone-mc, mbeacom and SaptakS August 6, 2018 13:12

stickler-ci reviewed Aug 6, 2018

View reviewed changes

darkdarkdragon force-pushed the 1807_allow_auto_approve_off branch from 8d538da to 3b75809 Compare August 6, 2018 23:31

darkdarkdragon force-pushed the 1807_allow_auto_approve_off branch from eeccce5 to 90f5749 Compare August 9, 2018 14:08

mbeacom suggested changes Aug 10, 2018

View reviewed changes

darkdarkdragon force-pushed the 1807_allow_auto_approve_off branch 2 times, most recently from 2b89996 to 03812a5 Compare August 24, 2018 00:20

darkdarkdragon force-pushed the 1807_allow_auto_approve_off branch from eaeb99c to a0bb203 Compare September 9, 2018 23:04

thelostone-mc previously approved these changes Sep 10, 2018

View reviewed changes

mbeacom dismissed thelostone-mc’s stale review via 53f1198 September 24, 2018 13:38

mbeacom previously approved these changes Sep 24, 2018

View reviewed changes

SaptakS suggested changes Sep 24, 2018

View reviewed changes

mbeacom self-requested a review September 24, 2018 15:43

darkdarkdragon dismissed mbeacom’s stale review via 4dccb0d October 2, 2018 07:53

darkdarkdragon force-pushed the 1807_allow_auto_approve_off branch from c3cc11b to 4dccb0d Compare October 2, 2018 07:53

allow funder to turn off auto approvals at bounty creation

4e97e68

darkdarkdragon force-pushed the 1807_allow_auto_approve_off branch from 4dccb0d to 4e97e68 Compare October 2, 2018 07:59

pinkiebell reviewed Oct 12, 2018

View reviewed changes

app/dashboard/templates/shared/issue_details.html Show resolved Hide resolved

mbeacom approved these changes Nov 8, 2018

View reviewed changes

mbeacom merged commit 184d4d0 into gitcoinco:master Nov 8, 2018

allow funder to turn off auto approvals at bounty creation #1945

allow funder to turn off auto approvals at bounty creation #1945

Conversation

darkdarkdragon commented Aug 5, 2018

Description

Checklist

Testing

Refers/Fixes

Choose a reason for hiding this comment

codecov bot commented Aug 5, 2018 • edited Loading

Codecov Report

PixelantDesign commented Aug 6, 2018

darkdarkdragon commented Aug 6, 2018

darkdarkdragon commented Aug 6, 2018

darkdarkdragon commented Aug 6, 2018

Choose a reason for hiding this comment

darkdarkdragon commented Aug 6, 2018

Choose a reason for hiding this comment

thelostone-mc commented Aug 7, 2018

PixelantDesign commented Aug 7, 2018 • edited Loading

darkdarkdragon commented Aug 7, 2018

PixelantDesign commented Aug 7, 2018

PixelantDesign commented Aug 9, 2018

darkdarkdragon commented Aug 9, 2018

thelostone-mc commented Aug 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frankchen07 commented Aug 15, 2018

darkdarkdragon commented Aug 16, 2018

thelostone-mc commented Sep 6, 2018

stickler-ci commented Sep 9, 2018

darkdarkdragon commented Sep 9, 2018

thelostone-mc left a comment

Choose a reason for hiding this comment

PixelantDesign commented Sep 24, 2018

mbeacom commented Sep 24, 2018

mbeacom commented Sep 24, 2018

SaptakS left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darkdarkdragon commented Oct 2, 2018

pinkiebell left a comment

Choose a reason for hiding this comment

PixelantDesign commented Oct 18, 2018 • edited Loading

mbeacom left a comment

Choose a reason for hiding this comment

codecov bot commented Aug 5, 2018 •

edited

Loading

PixelantDesign commented Aug 7, 2018 •

edited

Loading

PixelantDesign commented Oct 18, 2018 •

edited

Loading