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

#4745: Surface re-marketed issues to the top of the issue explorer #4876

Merged
merged 2 commits into from
Aug 14, 2019

Conversation

vince0656
Copy link
Contributor

@vince0656 vince0656 commented Jul 26, 2019

Description

Changes meet the requirements detailed in issue #4745. The new code adds the ability to re-market issues that have been abandoned or had all workers rejected. When issues are re-marketed, the new code surfaces the issue to the top of the issue explorer under a new default sorting option ‘Last Marketed’. In addition, the new code will:
-Not allow a funder to market an issue more than twice
-Send a slack notification to #notif
-Re-order buttons on the bounty details page into a more convenient drop down

Refers/Fixes

#4745

Testing

Appropriate unit tests have been included

Video demonstration

https://drive.google.com/open?id=1J_eGGdcepsupj6yx3KYV1WqHUmcFR2Lv

Please bear in mind that my laptop slowed down during recording which is why the first re-market took a while to kick in :)

@vince0656
Copy link
Contributor Author

Reason for travis failure:

E django.core.management.base.CommandError: Conflicting migrations detected; multiple leaf nodes in the migration graph: (0043_auto_20190724_1507, 0042_auto_20190718_1749 in dashboard).
E To fix them run 'python manage.py makemigrations --merge'

@codecov
Copy link

codecov bot commented Jul 27, 2019

Codecov Report

Merging #4876 into master will decrease coverage by 12.77%.
The diff coverage is 6.66%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #4876       +/-   ##
===========================================
- Coverage   30.64%   17.87%   -12.78%     
===========================================
  Files         216      205       -11     
  Lines       17407    17374       -33     
  Branches     2363     2370        +7     
===========================================
- Hits         5335     3106     -2229     
- Misses      11857    14259     +2402     
+ Partials      215        9      -206
Impacted Files Coverage Δ
app/dashboard/utils.py 16.87% <ø> (-17.62%) ⬇️
app/dashboard/helpers.py 11.89% <ø> (-2.43%) ⬇️
app/dashboard/router.py 0% <0%> (-34.67%) ⬇️
app/dashboard/models.py 38.28% <16.66%> (-18.16%) ⬇️
app/dashboard/views.py 11.08% <3.44%> (-3.22%) ⬇️
app/kudos/forms.py 0% <0%> (-100%) ⬇️
app/grants/serializers.py 0% <0%> (-100%) ⬇️
app/avatar/serializers.py 0% <0%> (-100%) ⬇️
app/grants/urls.py 0% <0%> (-100%) ⬇️
app/inbox/urls.py 0% <0%> (-100%) ⬇️
... and 83 more

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 f9ae26f...4423e5e. Read the comment docs.

@codecov
Copy link

codecov bot commented Jul 27, 2019

Codecov Report

Merging #4876 into master will decrease coverage by <.01%.
The diff coverage is 31.11%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4876      +/-   ##
==========================================
- Coverage   30.93%   30.93%   -0.01%     
==========================================
  Files         217      217              
  Lines       17413    17456      +43     
  Branches     2381     2392      +11     
==========================================
+ Hits         5387     5400      +13     
- Misses      11807    11836      +29     
- Partials      219      220       +1
Impacted Files Coverage Δ
app/dashboard/utils.py 37.12% <ø> (ø) ⬆️
app/dashboard/helpers.py 14.21% <ø> (ø) ⬆️
app/dashboard/router.py 34.51% <25%> (-0.31%) ⬇️
app/dashboard/views.py 14.02% <3.57%> (-0.21%) ⬇️
app/dashboard/models.py 57.02% <92.3%> (+0.26%) ⬆️

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 a0b5de8...c9eba17. Read the comment docs.

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.

@vince0656 glad to you have you contributing but could you update the PRs description + title with what exactly it does + a recording of the fix
This enables

  • the funder + PR reviews to get more context on what exactly it solves
  • makes reviewing easier
  • and we enforce this for every PR which gets in + it's just good practice ^_^

Codewise looks alright :)

@vince0656 vince0656 changed the title Fixes: https://github.com/gitcoinco/web/issues/4745 #4745: Surface re-marketed issues to the top of the issue explorer Jul 27, 2019
@vince0656
Copy link
Contributor Author

Whilst trying to record a video of my feature working in action, I found a bug which prevented me from loading the bounty details page. Bug was introduced in #4775 on line 135 and 152 of app/assets/v2/js/pages/bounty_details.js - I have fixed this in the latest push

@vince0656 vince0656 force-pushed the remarket-resurface branch from b4ed8b8 to 2d0acaa Compare July 28, 2019 11:56
@vince0656
Copy link
Contributor Author

@thelostone-mc I have addressed all points in your comment and have attached a link to a video demonstrating the new feature.

Any further questions, please don't hesitate to ask

thelostone-mc
thelostone-mc previously approved these changes Jul 28, 2019
PixelantDesign
PixelantDesign previously approved these changes Jul 31, 2019
@PixelantDesign
Copy link
Contributor

PixelantDesign commented Aug 1, 2019

Looking great @vince0656!

My only change request is on the confirmation banner.

1st Remarket Please update to:

This issue has been remarketed. The issue will appear at top of the issue explorer. You will be able to remarket this bounty one more time if a contributor does not pick this up.

2nd Remarket

This issue has been remarketed. The issue will appear at the top of the issue explorer. Please note this is the last time the issue is able to me remarketed.

Thanks! Please let me know once this copy is in!

@vince0656 vince0656 dismissed stale reviews from PixelantDesign and thelostone-mc via 163b255 August 1, 2019 16:52
@vince0656
Copy link
Contributor Author

@PixelantDesign the copy changes you requested have been pushed in the latest commit.

Thanks,
Vincent

@vince0656 vince0656 force-pushed the remarket-resurface branch from 163b255 to 2f4e68c Compare August 1, 2019 17:34
@vince0656
Copy link
Contributor Author

@PixelantDesign build issues have now been rectified

@PixelantDesign
Copy link
Contributor

Great! @thelostone-mc will you take a look please? Thanks!

@vince0656
Copy link
Contributor Author

@thelostone-mc I am just in the process of resolving the conflicts that will block merging

@vince0656 vince0656 force-pushed the remarket-resurface branch from 2f4e68c to f4e908f Compare August 8, 2019 19:23
thelostone-mc
thelostone-mc previously approved these changes Aug 10, 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