Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change Bounty Details #1968

Merged
merged 3 commits into from
Aug 27, 2018
Merged

Change Bounty Details #1968

merged 3 commits into from
Aug 27, 2018

Conversation

pinkiebell
Copy link
Contributor

@pinkiebell pinkiebell commented Aug 8, 2018

bounty details ('experience_level', 'project_length', 'bounty_type',
'permission_type', 'project_type')

can be changed if the bounty is still on 'the happy path' by the funder
or staff.

Fixes #1928

@@ -0,0 +1,72 @@

// Overwrite from shared.js
function trigger_form_hooks() {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unexpected empty function 'trigger_form_hooks'. (no-empty-function)


// Overwrite from shared.js
function trigger_form_hooks() {
};

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary semicolon. (no-extra-semi)

"""Change bounty."""


if request.body:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

E303 too many blank lines (2)

@codecov
Copy link

codecov bot commented Aug 8, 2018

Codecov Report

Merging #1968 into master will decrease coverage by 0.2%.
The diff coverage is 4.93%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1968      +/-   ##
==========================================
- Coverage   28.11%   27.91%   -0.21%     
==========================================
  Files         136      136              
  Lines       11025    11130     +105     
  Branches     1475     1499      +24     
==========================================
+ Hits         3100     3107       +7     
- Misses       7816     7913      +97     
- Partials      109      110       +1
Impacted Files Coverage Δ
app/app/urls.py 84.09% <ø> (ø) ⬆️
app/dashboard/notifications.py 16.76% <0%> (-0.33%) ⬇️
app/dashboard/helpers.py 17.2% <0%> (ø) ⬆️
app/retail/emails.py 20.18% <20%> (-0.01%) ⬇️
app/dashboard/views.py 14% <4.08%> (-0.74%) ⬇️
app/marketing/mails.py 11.52% <6.66%> (-0.17%) ⬇️
app/app/settings.py 78.57% <0%> (-4.77%) ⬇️
app/dashboard/models.py 50.23% <0%> (-0.16%) ⬇️
.../management/commands/pending_start_work_actions.py 0% <0%> (ø) ⬆️

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 b64b504...537ac16. Read the comment docs.

@pinkiebell
Copy link
Contributor Author

Any feedback on this one? 😄

@owocki
Copy link
Contributor

owocki commented Aug 17, 2018

it still says WIP in the PR title and theres a few TODOs in the diff. is this ready for review?

@pinkiebell
Copy link
Contributor Author

Yes, it's WIP because of the comments in the PR. I asked for first review in the linked issue ;)

@owocki
Copy link
Contributor

owocki commented Aug 20, 2018

QA notes

  1. doesnt seem like the user can edit any of the details around project type
  2. why is the URL for 'change details' so long? example URL. is it to transfer data from view to view? if so, why not just add ?pk=<pk> to the view and load the appro info from the database?
  3. i dont think we want to let the user edit their issue desc / title on this view... i'd rather point them to github.
  4. submit button on this page doesnt seem to do anything.. nor does their seem to be any validation on the page..

happ y to QA more when the items in the definition of done on the spec are completed

@@ -754,6 +754,8 @@ var do_actions = function(result) {
const show_suspend_auto_approval = document.isStaff && result['permission_type'] == 'approval';
const show_admin_methods = document.isStaff;
const show_moderator_methods = document.isModerator;
const show_change_bounty = isBountyOwner(result);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or is_admin

</head>

<body class="interior submit_bounty" style="background-color: #FFFFFF;">
{% include 'shared/tag_manager_2.html' %}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why build a whole new form. why not just re-use the new_bounty form?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I take a look at the new changes and modify the template if necessary for easier reuse. 👍

timezone.datetime.fromtimestamp(deadline),
timezone=UTC)

item = params.get('attached_job_description', '')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i wonder if we could use django forms for any of this repetitive code.... cc @mbeacom


<h1>{% trans "Bounty Details Changed" %}</h1>
<br>

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we list out the details that have changed.. ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don' think that would be needed! cause the funder would see it on the bounty page upon redirection

@@ -0,0 +1,3 @@
Bounty Details Changed

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no data in this email

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I included the bounty.txt template 😕

@pinkiebell
Copy link
Contributor Author

pinkiebell commented Aug 21, 2018

@owocki

Thanks for review 👍

  1. doesnt seem like the user can edit any of the details around project type

That's odd, seems to be a conflict with new changes on master since I worked on it.
Will fix.

  1. why is the URL for 'change details' so long? example URL. is it to transfer data from view to view?
    if so, why not just add ?pk= to the view and load the appro info from the database?

Yep, I transfer the bounty details with this method.
I will change it to use the pk= query parameter.

  1. i dont think we want to let the user edit their issue desc / title on this view... i'd rather point them to github.

That I wasn't sure about. Will be removed.

  1. submit button on this page doesnt seem to do anything.. nor does their seem to be any validation on the page..

The same problem like 1.

keys = ['experience_level', 'project_length', 'bounty_type', 'permission_type', 'project_type']

if request.body:
can_change = (bounty.status in Bounty.OPEN_STATUSES) or (bounty.can_submit_after_expiration_date and bounty.status is 'expired')

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

E501 line too long (136 > 120 characters)

@pinkiebell pinkiebell changed the title WIP: Change Bounty Details Change Bounty Details Aug 21, 2018
@pinkiebell
Copy link
Contributor Author

@owocki

Ready for review (again) 👍 .
The only open question is the additional content of the email templates; if wanted.

@owocki
Copy link
Contributor

owocki commented Aug 22, 2018

just tested... seems to work well 👍

owocki
owocki previously approved these changes Aug 22, 2018
@thelostone-mc
Copy link
Member

@pinkiebell quick rebase and then we merge ?

bounty details ('experience_level', 'project_length', 'bounty_type',
'permission_type', 'project_type')

can be changed if the bounty is still on 'the happy path' by the funder
or staff.

Fixes #1928
@pinkiebell
Copy link
Contributor Author

@thelostone-mc Done! 👍

thelostone-mc
thelostone-mc previously approved these changes Aug 24, 2018
Copy link
Member

@thelostone-mc thelostone-mc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tested it out ! LGTM

@thelostone-mc thelostone-mc added the enhancement This is a feature to be enhanced or improved. label Aug 24, 2018
@thelostone-mc
Copy link
Member

@PixelantDesign this is how it looks
x

@pinkiebell Oh i just noticed should we update the placeholder text ?
Along the lines to -> Update your bounty settings to get the right crowd ? @vs77bb might have something better :P

@pinkiebell
Copy link
Contributor Author

@thelostone-mc
Did you mean the button on the bounty details page or on the bounty change page?
Instead of 'Details'- at the top?

@thelostone-mc
Copy link
Member

@pinkiebell the button on the bounty details page

@pinkiebell
Copy link
Contributor Author

@thelostone-mc Done 👍

@thelostone-mc
Copy link
Member

thelostone-mc commented Aug 24, 2018

sweet!! will merge this in once @PixelantDesign reviews it 👍

Copy link
Contributor

@PixelantDesign PixelantDesign left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good @pinkiebell!

On the buttons, could we change....

  1. 'Change Bounty Details' ----> 'Edit Issue Details'
  2. Change Details Screen...could we change the commit button to "Update Issue Details"

Nice work!

@PixelantDesign
Copy link
Contributor

My other question is, if a funder is updating the issue details..should the following also happen...

  1. Gitcoin Bot should update the thread with the revised details.
  2. The funder should receive a confirmation email of the change.
  3. Anyone that has Started Work on the issue should also receive an email confirmation.

Thoughts from the group? @owocki @vs77bb @thelostone-mc @pinkiebell.

@pinkiebell
Copy link
Contributor Author

@PixelantDesign
Copy update done!
Regarding your questions, 2. and 3. are already the case with this PR.
For number 1. any ideas how that should look like?

@vs77bb
Copy link
Contributor

vs77bb commented Aug 27, 2018

LGTM!

Gitcoin Bot should update the thread with the revised details.

I think we should hold off on this for now. There's already enough comments on the Github thread, IMO. If it turns out to be an issue, we could include 'Approval' vs. 'Permissions' bounties in a v2 of the PR @oogetyboogety is working on regarding the Gitcoin Bot copy updates!

@PixelantDesign
Copy link
Contributor

sounds good. Let's merge @thelostone-mc @SaptakS ?

@thelostone-mc thelostone-mc merged commit 2f65f25 into gitcoinco:master Aug 27, 2018
@thelostone-mc
Copy link
Member

@pinkiebell thanks for the contribution ^_^

@vs77bb
Copy link
Contributor

vs77bb commented Aug 28, 2018

Sweeeeeeeeeetttt!

@pinkiebell pinkiebell deleted the bounty_changed branch November 6, 2018 08:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This is a feature to be enhanced or improved.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants