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

bug: handle where fee is 0 for bounty creation + increase funding #4285

Merged
merged 7 commits into from
May 2, 2019

Conversation

thelostone-mc
Copy link
Member

@thelostone-mc thelostone-mc commented Apr 25, 2019

Description

This PR ensures that bounty creation and increase funding both work as expected when fee is set to 0.
The fee section is hidden when the fee is set to 0, that way the user doesn't see the banner making it less confusing. The total amount, however, states we charge no fees to make the user know that we are not charging them anything extra. The same thing in increase funding looks like THIS

Getting a lil more detailed

  • updated code to hide fee section when fee is 0 & update content
    as needed.
  • avoid extra metamask popup for fee when fee is 0.
  • code refactor + css prettify

Tests Done

Bounty creation
  • No fee + Featured Bounty
  • No fee + Non-Featured Bounty
  • Fee + Featured Bounty
  • No fee + Non-Featured Bounty
Bounty creation
  • No fee + increase funding
  • With fee + increase funding

Yes I tested this like a crazy person.

img: see here

cc @frankchen07

- updated code to hide fee section when fee is 0 & update content
  as needed.
- avoid extra metamask popup for fee when fee is 0
- code refactor + css prettify
- updated code to hide fee section when fee is 0 & update content
  as needed.
- avoid extra metamask popup for fee when fee is 0
- code refactor + css prettify
@thelostone-mc thelostone-mc requested a review from owocki April 25, 2019 18:57
@thelostone-mc thelostone-mc added bug This is something that isn't working as intended. Gitcoin Bounties Gitcoin Bounties labels Apr 25, 2019
@thelostone-mc thelostone-mc changed the title bounty/new: handle where fee is 0 bug: handle where fee is 0 for bounty creation + increase funding Apr 25, 2019
@codecov
Copy link

codecov bot commented Apr 25, 2019

Codecov Report

Merging #4285 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #4285   +/-   ##
======================================
  Coverage    30.2%   30.2%           
======================================
  Files         209     209           
  Lines       16776   16776           
  Branches     2245    2245           
======================================
  Hits         5067    5067           
  Misses      11519   11519           
  Partials      190     190

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 1faef16...559995e. Read the comment docs.

Copy link
Contributor

@danlipert danlipert left a comment

Choose a reason for hiding this comment

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

Looks pretty good! just a few questions and a typo fix :)

@@ -36,7 +39,7 @@ $(document).ready(function() {
var denomSelect = $('select[name=denomination]');

localStorage['tokenAddress'] = denomSelect.data('tokenAddress');
localStorage['amount'] = 0;
localStorage['amount'] = 0.01;
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this change do?

Copy link
Member Author

Choose a reason for hiding this comment

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

This ensures that the min amount is set to 0.01 as opposed to 0.
I originally updated on the html tag but the value here over-writes that, so had to udpate this too

In Increase funding amount shouldn't be set to 0 during submit.
If the amount is defaulted 0 -> the fee banner doesn't make sense cause it would say we charge 10% fee which is 0 :P

we set default amount to > 0 so that we don't show 10% fee which equates to 0.

if (error) {
_alert({ message: gettext('Unable to pay bounty fee. Please try again.') }, 'error');
} else {
// TODO: Save txnId + feeamount to bounty;
Copy link
Contributor

Choose a reason for hiding this comment

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

does this TODO still need to be added?

Copy link
Member Author

Choose a reason for hiding this comment

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

This would be a separate task!
This would involve updating the bounty model / creating a new model to keep track of the fees.

I had brought this up when we originally built out fee%.
As of now we have no means to store and track it.
Here I've simple reorganized the code

need to evaluate if it would sense to keep fee in it's model and map bounties and other produxts to it. cc @owocki

app/assets/v2/js/pages/new_bounty.js Outdated Show resolved Hide resolved
app/dashboard/templates/bounty/increase.html Outdated Show resolved Hide resolved
app/dashboard/templates/bounty/new.html Outdated Show resolved Hide resolved
@thelostone-mc thelostone-mc requested a review from danlipert April 27, 2019 09:28
@thelostone-mc
Copy link
Member Author

@danlipert fixed up the typo + updated the event for no fee scenario
left the comments for the other two ^_^

@danlipert danlipert merged commit 692a11d into master May 2, 2019
@thelostone-mc thelostone-mc deleted the dynamic_fee branch July 4, 2019 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This is something that isn't working as intended. Gitcoin Bounties Gitcoin Bounties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants