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

Integrate StandardBounties increasePayout() #626

Merged
merged 1 commit into from
Mar 26, 2018

Conversation

msuess
Copy link
Contributor

@msuess msuess commented Mar 14, 2018

Description

Gives the funder the ability to increase the payout.

Changes:

  • add new route for increase_bounty view
  • add increase_bounty view and js
  • DRY up duplicate code in new_bounty.js and increase_bounty.js
  • implement bounty increase notification through git, slack and twitter
Checklist
  • linter status: 100% pass
  • changes don't break existing behavior
  • commit message follows commit guidelines
Affected core subsystem(s)
  • ui
Testing

Testing in progress

Refers/Fixes

Fixes #617

@msuess
Copy link
Contributor Author

msuess commented Mar 14, 2018

This currently only works for ETH. Do we need to support tokens as well?

@mbeacom mbeacom changed the title Integrate StandardBounties increasePayout() WIP: Integrate StandardBounties increasePayout() Mar 14, 2018
@mbeacom mbeacom added backend This needs backend expertise. bounties wip labels Mar 14, 2018
@msuess
Copy link
Contributor Author

msuess commented Mar 15, 2018

Implemented token support & did some cleanup and testing. The basic functionality is done. It should not interfere with other functionality, however I'm not sure about all the possible side-effects caused by localStorage.

I did not touch the github bot or the email functionality. Is there a need for notifications about bounty increases through these channels?

I haven't fixed the linter errors yet - I'm happy to do that, squash, and finalize the PR once you guys are happy with my changes.

Let me know if I'm missing anything :)

@owocki
Copy link
Contributor

owocki commented Mar 15, 2018

I did not touch the github bot or the email functionality. Is there a need for notifications about bounty increases through these channels?

yes please! here is the part of the system that can/should notice the increase in funds and then trigger a notification. https://github.com/gitcoinco/web/blob/master/app/dashboard/helpers.py#L551-L552

I haven't fixed the linter errors yet - I'm happy to do that, squash, and finalize the PR once you guys are happy with my changes.

its pretty easy to just update the PR with isort whenever!

parent: 'right_actions',
color: enabled ? 'darkBlue' : 'darkGrey',
extraClass: enabled ? '' : 'disabled',
title: enabled ? 'Increase the funding of this bounty' : 'Can only be performed if you are the funder.'
Copy link
Contributor

Choose a reason for hiding this comment

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

cant wait to get a v2 out there so that we can remove Can only be performed if you are the funder!

@@ -0,0 +1,160 @@
load_tokens();
var setUsdAmount= function (event) {
Copy link
Contributor

Choose a reason for hiding this comment

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

itd be great to DRY this (maybe move it into shared.js)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On it.

@owocki
Copy link
Contributor

owocki commented Mar 15, 2018

did not test but the code looks sane to me.
@mbeacom what do you think?
@msuess how did you test this? did you actually see the updated information on the funding details page after submitting an increase in funds?

_alert({ message: "Please enter a valid github issue URL." });
isError = true;
}
if(amount == ''){
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe worth it to add an error IFF someone tries to do this thats not the original funder

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do.

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'm doing this here, do you want me to move it?

}

function approveSuccessCallback () {
bounty.increasePayout(
Copy link
Contributor

Choose a reason for hiding this comment

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

i think youll need to batch up, and call the contribute() method here too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It worked during manual testing, but if you want I can do that. Out of curiosity: What makes you think that this is necessary?

Copy link
Contributor

Choose a reason for hiding this comment

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

@mbeylin told me it was :P

Copy link
Contributor

Choose a reason for hiding this comment

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

i believe per the standardbounties docs, those two methods complement each other https://github.com/ConsenSys/StandardBounties/

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'll look into it.

@msuess
Copy link
Contributor Author

msuess commented Mar 15, 2018

@owocki I tested it in my docker instance connected to rinkeby and saw the updated amounts on the bounty details page.

I'll look into the notifications.

@msuess
Copy link
Contributor Author

msuess commented Mar 15, 2018

Added notifications. I'm skipping the email for now since in v1 only the funder can increase bounties and the current email notification solution based on keywords does not scale.

@msuess msuess force-pushed the stdbounties-increasepayout branch from 395f984 to 2c45a58 Compare March 15, 2018 17:22
@owocki
Copy link
Contributor

owocki commented Mar 15, 2018

Added notifications. I'm skipping the email for now since in v1 only the funder can increase bounties and the current email notification solution based on keywords does not scale.

Makes senes to me.. I think a github/slack notification may be all we need at this point

@@ -239,6 +244,14 @@ def build_github_notification(bounty, event_name, profile_pairs=None):
f"[here]({absolute_url})\n * Questions? Get help on the " \
f"<a href='https://gitcoin.co/slack'>Gitcoin Slack</a>\n * ${amount_open_work}" \
" more Funded OSS Work Available at: https://gitcoin.co/explorer\n"
if event_name == 'increased_bounty':
Copy link
Contributor

Choose a reason for hiding this comment

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

awesome.

@msuess msuess force-pushed the stdbounties-increasepayout branch from bd97ccf to 09b34f3 Compare March 15, 2018 23:14
@msuess
Copy link
Contributor Author

msuess commented Mar 15, 2018

@mbeylin Looking into the StandardBounties docs, it seems like contribute() can not be called for bounties in active state. Since the bounties on gitcoin are activated on creation via issueAndActivateBounty, I can't call the contribute() method in batch with increasePayout(), however, everything seems to work fine and the bounty increases accordingly. Any ideas? What makes you think that calling contribute() is necessary in this case?

@msuess msuess force-pushed the stdbounties-increasepayout branch 2 times, most recently from 4ef1f5a to 373bcab Compare March 16, 2018 16:03
@codecov
Copy link

codecov bot commented Mar 16, 2018

Codecov Report

Merging #626 into master will decrease coverage by 0.07%.
The diff coverage is 13.04%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #626      +/-   ##
==========================================
- Coverage   34.73%   34.66%   -0.08%     
==========================================
  Files          97       97              
  Lines        5317     5340      +23     
  Branches      597      602       +5     
==========================================
+ Hits         1847     1851       +4     
- Misses       3402     3418      +16     
- Partials       68       71       +3
Impacted Files Coverage Δ
app/app/urls.py 100% <ø> (ø) ⬆️
app/dashboard/helpers.py 31.22% <0%> (-0.25%) ⬇️
app/dashboard/views.py 17.9% <5.88%> (-0.6%) ⬇️
app/dashboard/notifications.py 15.38% <50%> (+0.77%) ⬆️

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 b848e9c...546cb85. Read the comment docs.

@msuess msuess force-pushed the stdbounties-increasepayout branch 5 times, most recently from feb37a0 to 6bb795a Compare March 16, 2018 19:21
@mbeylin
Copy link

mbeylin commented Mar 19, 2018

@msuess so you've actually discovered a hole in our documentation. You CAN call contribute() when the bounty is in any stage, as long as it isn't dead.

Also, if you are calling increasePayout(), there's no need to contribute, since you can send extra tokens along with the increase call without needing to batch them.

@msuess msuess force-pushed the stdbounties-increasepayout branch 2 times, most recently from 1671b1c to cc310dd Compare March 19, 2018 21:06
@mbeacom mbeacom changed the title WIP: Integrate StandardBounties increasePayout() Integrate StandardBounties increasePayout() Mar 23, 2018
@@ -476,6 +476,34 @@ def fulfill_bounty(request):

return TemplateResponse(request, 'fulfill_bounty.html', params)

def increase_bounty(request):
"""Increase a bounty (funder)"""
issueURL = request.GET.get('source')
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you camel_case this variable? Maybe: issue_url ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure!

app/app/urls.py Outdated
@@ -71,7 +71,7 @@
url(r'^issue/(?P<ghuser>.*)/(?P<ghrepo>.*)/(?P<ghissue>.*)', dashboard.views.bounty_details, name='issue_details_new2'),
url(r'^bounty/details/?', dashboard.views.bounty_details, name='bounty_details'),
url(r'^funding/details/?', dashboard.views.bounty_details, name='funding_details'),
url(r'^legacy/funding/details/?', dashboard.views.bounty_details, name='legacy_funding_details'),
Copy link
Contributor

Choose a reason for hiding this comment

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

🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

huh, I could have sworn this was removed at some point on master, will put it back

@@ -0,0 +1,109 @@
{% comment %}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you switch this file to use 2-space indentation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will do.

@msuess msuess force-pushed the stdbounties-increasepayout branch 2 times, most recently from 2acd253 to 0e26f9a Compare March 23, 2018 19:02
@msuess
Copy link
Contributor Author

msuess commented Mar 23, 2018

@mbeacom rebased and implemented the changes, feel free to check again :)

@msuess
Copy link
Contributor Author

msuess commented Mar 23, 2018

Travis says:

/home/travis/build/gitcoinco/web/app/assets/v2/js/pages/offchain_bounties.js
  3:5  error  Expected blank line after variable declarations  newline-after-var

I think this is something on master, I didn't touch this file.

@msuess
Copy link
Contributor Author

msuess commented Mar 23, 2018

@owocki @mbeacom I'll rebase again once the build on master is fixed.

@mbeacom
Copy link
Contributor

mbeacom commented Mar 23, 2018

@msuess Master should be good to go now.

Gives the funder the ability to increase the payout.

Changes:
- add new route for increase_bounty view
- add increase_bounty view and js
- DRY up duplicate code in new_bounty.js and increase_bounty.js
- implement bounty increase notification through git, slack and twitter
@msuess msuess force-pushed the stdbounties-increasepayout branch from 0e26f9a to 546cb85 Compare March 23, 2018 19:35
@msuess
Copy link
Contributor Author

msuess commented Mar 23, 2018

@mbeacom rebased!

Copy link
Contributor

@mbeacom mbeacom left a comment

Choose a reason for hiding this comment

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

lgtm - Thanks for the contribution @msuess ! I'm going to deploy this to https://stage.gitcoin.co and test it out.

@msuess
Copy link
Contributor Author

msuess commented Mar 23, 2018

Thanks @mbeacom!

@mbeacom
Copy link
Contributor

mbeacom commented Mar 23, 2018

Tested on staging. Everything worked appropriately. 🙌

@msuess
Copy link
Contributor Author

msuess commented Mar 23, 2018

Nice, glad to hear that @mbeacom!

@msuess
Copy link
Contributor Author

msuess commented Mar 26, 2018

@mbeacom @owocki is there anything else you need from me in order to merge this?

@owocki
Copy link
Contributor

owocki commented Mar 26, 2018

testing now

@owocki
Copy link
Contributor

owocki commented Mar 26, 2018

notes:

minor

  1. on the funding/increase page, the default gas limit in the metamask preview is 0. this is wrong, it should be 56269.
  2. the step for the 'amount' field on the funding/increase page should be 0.001, not 1
  3. there should be some padding here => http://bits.owocki.com/1r0K0n453J3t/Screen%20Shot%202018-03-26%20at%209.41.29%20AM.png

major

i tried funding an issue 3x with 0.01 ETH each time. each time the UI reflected correctly an updated value.

but unfortunately, when i finally submitted an accepted work on the bounty, no funds were transfered to the submitter on the ether tx and the contract retained my 0.03 ETH

funding txs:

  1. https://rinkeby.etherscan.io/tx/0x09dddf78fc34a8206118479395cdeb92dfed00ff2a0f1e746995635bda6296eb
  2. https://rinkeby.etherscan.io/tx/0x8da38f5d07d99724609ba9a37078d806ff77ba9b027e2b8b61da810dee010506
  3. https://rinkeby.etherscan.io/tx/0x98b12caab6e4364149a99a4741a2be89241a7a6c3446311f2480d687e896f688

acceptance tx: https://rinkeby.etherscan.io/address/0xf209d2b723b6417cbf04c07e733bee776105a073

@owocki
Copy link
Contributor

owocki commented Mar 26, 2018

nevermind the above Major point. rinkeby.etherscan has an old interface and that's why it didnt appear as though the payouts were working

@owocki owocki mentioned this pull request Mar 26, 2018
3 tasks
@owocki owocki merged commit 90f0883 into gitcoinco:master Mar 26, 2018
@owocki
Copy link
Contributor

owocki commented Mar 26, 2018

@msuess false alarm!

@msuess
Copy link
Contributor Author

msuess commented Mar 26, 2018

@owocki Cheers, thanks for looking into this!

@gitcoinco gitcoinco deleted a comment from darkdarkdragon Jun 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend This needs backend expertise. bounties
Projects
None yet
4 participants