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

Reserve bounty improvements #5066

Merged
merged 6 commits into from
Sep 19, 2019

Conversation

vince0656
Copy link
Contributor

@vince0656 vince0656 commented Aug 24, 2019

Description

Inprovements to the reserve bounty functionality including a new 'reserved' state for bounties so that they don't show up on the issue explorer when users are searching for 'open' bounties

Refers/Fixes

#3544

Testing

Appropriate unit tests have been added

Video demonstration

https://drive.google.com/open?id=1-WZrPWReWAC2h6-u54FmU7phD9GX9iCA

@codecov
Copy link

codecov bot commented Aug 24, 2019

Codecov Report

Merging #5066 into master will decrease coverage by <.01%.
The diff coverage is 36%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5066      +/-   ##
==========================================
- Coverage   31.03%   31.02%   -0.01%     
==========================================
  Files         218      218              
  Lines       17515    17533      +18     
  Branches     2404     2409       +5     
==========================================
+ Hits         5436     5440       +4     
- Misses      11856    11870      +14     
  Partials      223      223
Impacted Files Coverage Δ
app/dashboard/router.py 34.72% <0%> (-1.21%) ⬇️
app/dashboard/helpers.py 14.01% <0%> (-0.21%) ⬇️
app/dashboard/models.py 56.7% <100%> (+0.09%) ⬆️

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 d1ede3a...ae56549. Read the comment docs.

@codecov
Copy link

codecov bot commented Aug 24, 2019

Codecov Report

Merging #5066 into master will increase coverage by 0.07%.
The diff coverage is 48.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5066      +/-   ##
==========================================
+ Coverage   30.71%   30.78%   +0.07%     
==========================================
  Files         221      221              
  Lines       17870    17941      +71     
  Branches     2469     2491      +22     
==========================================
+ Hits         5489     5524      +35     
- Misses      12155    12187      +32     
- Partials      226      230       +4
Impacted Files Coverage Δ
app/dashboard/router.py 34.72% <0%> (-1.21%) ⬇️
app/dashboard/helpers.py 13.94% <0%> (-0.21%) ⬇️
app/dashboard/views.py 14.12% <6.66%> (-0.08%) ⬇️
app/dashboard/utils.py 41.47% <77.77%> (+0.74%) ⬆️
app/dashboard/models.py 56.38% <78.94%> (+0.46%) ⬆️

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 2cadb0c...1957752. Read the comment docs.

@thelostone-mc
Copy link
Member

@vince0656 nice ! mind opening a draft PR going forward so that it's extra clear that the PR is not ready for review ?

@vince0656 vince0656 changed the title Reserve bounty improvements Draft PR - Reserve bounty improvements Aug 24, 2019
@vince0656
Copy link
Contributor Author

@vince0656 nice ! mind opening a draft PR going forward so that it's extra clear that the PR is not ready for review ?

Sure! I've updated the title. Is that sufficient or is there anything else I need to do to make it clear it's a draft PR?

@danlipert
Copy link
Contributor

@vince0656 Hey Vincent - when you create a new PR, theres an option to create it as a normal PR, or a draft PR. The difference is that it doesn't ping us for reviews until you set it ready for review if its a draft PR 👍

@vince0656 vince0656 force-pushed the reserve-bounty-improvements branch from e4dc0e8 to 10a45ff Compare August 28, 2019 20:49
@thelostone-mc
Copy link
Member

sweet looks like we making progress ^_^

@vince0656 vince0656 force-pushed the reserve-bounty-improvements branch 2 times, most recently from 5cd20ca to 5c2f1ef Compare August 29, 2019 18:33
@vince0656 vince0656 changed the title Draft PR - Reserve bounty improvements Reserve bounty improvements Aug 29, 2019
@vince0656
Copy link
Contributor Author

@danlipert @thelostone-mc @PixelantDesign All good to go 👍

delta = self.reserved_for_user_expiration - self.reserved_for_user_from
days = delta.days

if days > 0:
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a simpler way we can humanize the timedelta? https://anandnalya.com/2009/05/humanizing-the-time-difference-in-django/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey Dan. I'm not sure what you mean? I've had a look at the link you sent and we would still need to do calcs for hours. Additionally, the humanised timedelta outputs seem different compared to the current requirement of allowing a funder to reserve for '3 days', '1 week' or 'Keep reserved'. I tried to make the code as scalable as possible and struggled to find easier ways of doing this. It's a shame the timedelta object doesn't have an hours getter attribute or even a weeks one which is strange as you can construct a timedelta with weeks, days, hours, minutes etc.

Copy link
Contributor Author

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.

@danlipert any thoughts on my comment?

thelostone-mc
thelostone-mc previously approved these changes Sep 6, 2019
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.

me excitedddddd

@vince0656
Copy link
Contributor Author

I’m excited too! Any thoughts @danlipert?

app/assets/v2/css/submit_bounty.css Outdated Show resolved Hide resolved
@vince0656
Copy link
Contributor Author

@octavioamu Will fix the build issues when I get a chance

@vince0656 vince0656 force-pushed the reserve-bounty-improvements branch from 685a66c to 35c5141 Compare September 12, 2019 18:53
@vince0656
Copy link
Contributor Author

@octavioamu @thelostone-mc All good to go 👍

octavioamu
octavioamu previously approved these changes Sep 18, 2019
Copy link
Contributor

@octavioamu octavioamu left a comment

Choose a reason for hiding this comment

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

lgtm

thelostone-mc
thelostone-mc previously approved these changes Sep 19, 2019
@octavioamu octavioamu dismissed stale reviews from thelostone-mc and themself via d5d79d2 September 19, 2019 14:02
thelostone-mc
thelostone-mc previously approved these changes Sep 19, 2019
Copy link
Contributor

@octavioamu octavioamu left a comment

Choose a reason for hiding this comment

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

Just notice we need to recreate the migrations @vince0656

octavioamu
octavioamu previously approved these changes Sep 19, 2019
thelostone-mc
thelostone-mc previously approved these changes Sep 19, 2019
danlipert
danlipert previously approved these changes Sep 19, 2019
@thelostone-mc thelostone-mc dismissed stale reviews from danlipert, octavioamu, and themself via 08252d2 September 19, 2019 14:45
@thelostone-mc thelostone-mc force-pushed the reserve-bounty-improvements branch from cd37a16 to 08252d2 Compare September 19, 2019 14:45
@octavioamu octavioamu merged commit 04a588b into gitcoinco:master Sep 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants