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

Limit Profile stat results to valid bounties #791

Merged
merged 2 commits into from
Apr 4, 2018
Merged

Conversation

mbeacom
Copy link
Contributor

@mbeacom mbeacom commented Apr 4, 2018

Description

The goal of this PR is to allow filtering of bounties by a status field versus a property. This allows us to filter out unknown or cancelled bounties from the profile stats.

This PR introduces the Bounty.status field, renaming Bounty.status property to Bounty.current_status, migrates existing statuses, adds the stats_eligible queryset function, and makes use of it in Profile.stats.

Checklist
  • linter status: 100% pass
  • changes don't break existing behavior
  • commit message follows commit guidelines
Testing

Tested locally and on staging.

You can observe the state of these changes on: https://stage.gitcoin.co/profile/bradford-hamilton and compare against: https://gitcoin.co/profile/bradford-hamilton

@mbeacom mbeacom added backend This needs backend expertise. profile bounties labels Apr 4, 2018
@mbeacom mbeacom self-assigned this Apr 4, 2018
@mbeacom mbeacom requested a review from owocki April 4, 2018 18:12
@mbeacom
Copy link
Contributor Author

mbeacom commented Apr 4, 2018

@owocki I've updated the proposed changes to use idx_status since we're copying the return from Bounty.status property during pre_save anyway, so we're not introducing possible breaking changes or modifying all idx/status refs throughout the codebase.
scout

@codecov
Copy link

codecov bot commented Apr 4, 2018

Codecov Report

Merging #791 into master will increase coverage by 0.06%.
The diff coverage is 63.63%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #791      +/-   ##
==========================================
+ Coverage   33.79%   33.85%   +0.06%     
==========================================
  Files         101      101              
  Lines        5750     5757       +7     
  Branches      671      672       +1     
==========================================
+ Hits         1943     1949       +6     
- Misses       3728     3729       +1     
  Partials       79       79
Impacted Files Coverage Δ
app/dashboard/embed.py 6.22% <0%> (ø) ⬆️
app/dashboard/router.py 40.78% <100%> (+2.63%) ⬆️
app/dashboard/models.py 69.12% <66.66%> (-0.17%) ⬇️

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 de00083...e3a0e25. Read the comment docs.

@mkosowsk
Copy link

mkosowsk commented Apr 4, 2018

Code LGTM tho didn't test 👍🏻 thanks @mbeacom, this will be a nice fix so we don't harm stats for contributors for stuff that's outside of their control :)

@owocki
Copy link
Contributor

owocki commented Apr 4, 2018

@owocki I've updated the proposed changes to use idx_status since we're copying the return from Bounty.status property during pre_save anyway, so we're not introducing possible breaking changes or modifying all idx/status refs throughout the codebase.

okie dokie.. its a little less than elegant, but its still a step forward. makes sense to me

Copy link
Contributor

@owocki owocki left a comment

Choose a reason for hiding this comment

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

shippit

@mbeacom
Copy link
Contributor Author

mbeacom commented Apr 4, 2018

@owocki What would your ideal implementation be?

If you'd prefer, I can refactor the way we're determining the status, rename the field throughout, and modify the signaling. I just figured less changes in this PR was ideal. I am fine with making the changes in this branch, though.

@owocki
Copy link
Contributor

owocki commented Apr 4, 2018

@owocki What would your ideal implementation be?

prettymuch what you describe below, though i agree with your judgement call to not bother in retrospect.

If you'd prefer, I can refactor the way we're determining the status, rename the field throughout, and modify the signaling. I just figured less changes in this PR was ideal. I am fine with making the changes in this branch, though.

i say lets shippit once tested

@mbeacom mbeacom merged commit 0f3c688 into master Apr 4, 2018
@mbeacom mbeacom deleted the mark-status-changes branch April 4, 2018 21:44
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
Development

Successfully merging this pull request may close these issues.

3 participants