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.

Sign up for GitHub

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

Jump to bottom

Add filtering by applicants in bounties explorer #4613

Merged
merged 5 commits into from
Jul 3, 2019
Merged

Add filtering by applicants in bounties explorer #4613

merged 5 commits into from
Jul 3, 2019

Conversation

rafalkowalski
Copy link
Contributor

Fixes issue #4544

Kindly review
@thelostone-mc

image

Description
Refers/Fixes
Testing

@PixelantDesign
Copy link
Contributor

What is the default state?
We'd like the default to be 0 and the second option to be 1-5.

Thanks!

@rafalkowalski rafalkowalski force-pushed the i4544/add-filtering-bounties-by-applicants branch from dd3044a to 55280e7 Compare June 11, 2019 14:43
@codecov
Copy link

codecov bot commented Jun 11, 2019 •
edited
Loading

Codecov Report

Merging #4613 into master will decrease coverage by <.01%.
The diff coverage is 16.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4613      +/-   ##
==========================================
- Coverage   30.42%   30.41%   -0.01%     
==========================================
  Files         216      216              
  Lines       17234    17240       +6     
  Branches     2333     2335       +2     
==========================================
+ Hits         5243     5244       +1     
- Misses      11783    11788       +5     
  Partials      208      208
Impacted Files Coverage Δ
app/dashboard/router.py 32.25% <16.66%> (-0.45%) ⬇️

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 4205223...3e75156. Read the comment docs.

@rafalkowalski
Copy link
Contributor Author

@PixelantDesign I've set a default selection to 0

@thelostone-mc thelostone-mc requested a review from a team June 11, 2019 15:31
@thelostone-mc thelostone-mc added the Gitcoin Bounties Gitcoin Bounties label Jun 11, 2019
thelostone-mc
thelostone-mc previously approved these changes Jun 11, 2019
View reviewed changes
Copy link
Member

@thelostone-mc thelostone-mc left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM 🙌 @rafalkowalski could you ensure travis passes?

@rafalkowalski rafalkowalski force-pushed the i4544/add-filtering-bounties-by-applicants branch 2 times, most recently from e358bba to 7eb773e Compare June 11, 2019 15:57
@PixelantDesign
Copy link
Contributor

Thank you!

@octavioamu octavioamu requested a review from a team June 11, 2019 18:57
@octavioamu
Copy link
Contributor

Hey @rafalkowalski did you tested this in hackathon explorer? I think the filter will apply also there.

@rafalkowalski
Copy link
Contributor Author

@octavioamu Applicants filter isn't visible in hackathon explorer
image

danlipert
danlipert suggested changes Jun 12, 2019
View reviewed changes
app/dashboard/router.py Outdated Show resolved Hide resolved
@danlipert
Copy link
Contributor

@octavioamu Applicants filter isn't visible in hackathon explorer
image

@rafalkowalski Even though the filter isn't visible, the backend code that handles the request is the same. Since we store the filter in localStorage, we must make sure the same query parameter isn't submitted with the request on the hackathon page (I think this is what @octavioamu is referring to, we've had issues with this in the past)

@rafalkowalski rafalkowalski force-pushed the i4544/add-filtering-bounties-by-applicants branch from 7eb773e to 0677fee Compare June 12, 2019 19:32
@octavioamu
Copy link
Contributor

@rafalkowalski still failing in travis, there are some js lint errors run npm run eslint in your local to check it. Also update the branch with the last master changes.

@rafalkowalski rafalkowalski mentioned this pull request Jun 17, 2019
Closed
2 tasks
danlipert
danlipert previously approved these changes Jun 19, 2019
View reviewed changes
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 great! Can you please squash and rebase? Thanks!

@rafalkowalski rafalkowalski force-pushed the i4544/add-filtering-bounties-by-applicants branch from bf907f5 to d28dcaa Compare June 19, 2019 12:34
@rafalkowalski
Copy link
Contributor Author

@danlipert I've rebased it :)

thelostone-mc
thelostone-mc previously approved these changes Jun 19, 2019
View reviewed changes
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.

sweet @rafalkowalski ^_^

@rafalkowalski rafalkowalski force-pushed the i4544/add-filtering-bounties-by-applicants branch from d28dcaa to b3e8627 Compare June 21, 2019 16:13
@rafalkowalski rafalkowalski force-pushed the i4544/add-filtering-bounties-by-applicants branch from b3e8627 to 7d3c5db Compare June 24, 2019 13:12
octavioamu
octavioamu previously approved these changes Jun 24, 2019
View reviewed changes
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

@PixelantDesign
Copy link
Contributor

Hey guys! I just looked at some data and think it's best if we filter by 0 applicants as a default. I don't want to post the data here, but hit me up if you guys want to chat.

@rafalkowalski could we make the change to default to 0? Thank you!

@rafalkowalski rafalkowalski force-pushed the i4544/add-filtering-bounties-by-applicants branch from 7d3c5db to 9d080e7 Compare June 24, 2019 14:15
@thelostone-mc thelostone-mc merged commit c3d139a into gitcoinco:master Jul 3, 2019
owocki
owocki reviewed Jul 10, 2019
View reviewed changes
app/dashboard/router.py
@@ -267,6 +269,16 @@ def get_queryset(self):
statuses = self.request.query_params.get('status__in').split(',')
queryset = queryset.filter(idx_status__in=statuses)

applicants = self.request.query_params.get('applicants')
Copy link
Contributor

Choose a reason for hiding this comment

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

did you guys do any testing on performance here? this is a multi table JOIN between bounty + interest tables, and it increases the load time for the API on the explorer from 500ms to 10s+

Copy link
Contributor

Choose a reason for hiding this comment

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

note the diff between the applicants=0 calls here vs the ones that dont have that call in them...

Copy link
Contributor

Choose a reason for hiding this comment

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

the explorer page defaults to applicants=0, thus making most explorer lookups 10+s

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@owocki I will fix it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@owocki owocki owocki left review comments

@PixelantDesign PixelantDesign PixelantDesign left review comments

@thelostone-mc thelostone-mc thelostone-mc approved these changes

@octavioamu octavioamu octavioamu approved these changes

@danlipert danlipert danlipert approved these changes

Assignees
No one assigned
Labels
Gitcoin Bounties Gitcoin Bounties
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@rafalkowalski @PixelantDesign @octavioamu @danlipert @owocki @thelostone-mc