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

add 'stop work & slash' button #1343

Merged
merged 9 commits into from
Jun 12, 2018
Merged

add 'stop work & slash' button #1343

merged 9 commits into from
Jun 12, 2018

Conversation

jvmaia
Copy link
Contributor

@jvmaia jvmaia commented Jun 2, 2018

Description

implement the 'stop work & slash' feature

Refers/Fixes

closes #1340

@codecov
Copy link

codecov bot commented Jun 2, 2018

Codecov Report

Merging #1343 into master will decrease coverage by 0.02%.
The diff coverage is 28.57%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1343      +/-   ##
==========================================
- Coverage   30.27%   30.25%   -0.03%     
==========================================
  Files         125      125              
  Lines        9003     9041      +38     
  Branches     1156     1160       +4     
==========================================
+ Hits         2726     2735       +9     
- Misses       6169     6197      +28     
- Partials      108      109       +1
Impacted Files Coverage Δ
app/dashboard/views.py 15.6% <0%> (-0.15%) ⬇️
app/dashboard/models.py 56.54% <100%> (+0.03%) ⬆️
app/dashboard/router.py 31.73% <0%> (-1.95%) ⬇️
app/retail/utils.py 22.5% <0%> (-0.36%) ⬇️
app/marketing/management/commands/pull_stats.py 0% <0%> (ø) ⬆️
app/retail/views.py 39.41% <0%> (ø) ⬆️
app/dashboard/helpers.py 26.98% <0%> (+0.46%) ⬆️
app/dashboard/utils.py 29.09% <0%> (+0.71%) ⬆️

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 243190c...6e13c50. Read the comment docs.

@jvmaia
Copy link
Contributor Author

jvmaia commented Jun 4, 2018

The code is losing ~0.07% because the if/else statement in dashboard/views.py is extended. I can write it in one line, but i don't know if it would be a good idea.

@owocki
Copy link
Contributor

owocki commented Jun 4, 2018

reviewing now

@ghost ghost assigned owocki Jun 4, 2018
@ghost ghost added the in progress label Jun 4, 2018
owocki
owocki previously approved these changes Jun 4, 2018
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.

just tested. code looks good.. seems to be working @mbeacom what do you think

Copy link
Contributor

@mbeacom mbeacom left a comment

Choose a reason for hiding this comment

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

Generally, lgtm. Any reason for using an int in the url path versus a param?

@jvmaia
Copy link
Contributor Author

jvmaia commented Jun 6, 2018

Not a big game changer, but using a int in url we can reduce the size of the if/else statement inside views.uninterested().

var slashed = '0';
var success_message = 'Contributor removed from bounty.';

if (slash) {
Copy link
Contributor

@SaptakS SaptakS Jun 6, 2018

Choose a reason for hiding this comment

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

I think it's better to send true and false instead of 0 and 1. And then here you can write something like if slash == 'true' ? cc: @mbeacom

I feel int just reduces readibility and also there is no specific meaning of 0 and 1 from the url itslef. So I am not very comfortable with this.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with this. Additionally, we could probably swap this out to simply use a POST param versus the URL defined variable, defaulting to False if the request.POST param isn't present.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updating the code to use a POST param, as the @mbeacom said.

Copy link
Contributor Author

@jvmaia jvmaia Jun 6, 2018

Choose a reason for hiding this comment

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

Do you think it's better send the POST param only when the slash is true? Makes more sense to me always send it.

@mbeacom
Copy link
Contributor

mbeacom commented Jun 6, 2018

@owocki Are we married to "slash" here? Thoughts on alter_rep / ding_rep or something like that? After discussing it with @SaptakS, slash seems fairly misleading, since we use slash in multiple places to represent the presence of / and might prove difficult to maintain or approach by community members that are unfamiliar with the terminology.

Note: I understand the hunter wrote this to the spec, so I wouldn't expect him to modify this. Just something we might want to consider before we push it live.

@@ -347,7 +350,10 @@ def uninterested(request, bounty_id, profile_id):
bounty.interested.remove(interest)
maybe_market_to_slack(bounty, 'stop_work')
maybe_market_to_user_slack(bounty, 'stop_work')
event_name = "bounty_removed_by_staff" if is_staff else "bounty_removed_by_funder"
if is_staff:
event_name = "bounty_removed_slashed_by_staff" if request.POST['slashed'] == 'true' else "bounty_removed_by_staff"

Choose a reason for hiding this comment

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

E501 line too long (126 > 120 characters)

@@ -342,12 +345,16 @@ def uninterested(request, bounty_id, profile_id):
{'error': 'Only bounty funders are allowed to remove users!'},
status=401)

slashed = request.POST['slashed'] == 'true'
Copy link
Contributor

Choose a reason for hiding this comment

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

You could do: slashed = request.POST.get('slashed') here and you would only need to pass slashed if it's a reputation adverse event.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the tip!

app/app/urls.py Outdated
@@ -84,7 +84,7 @@
path('actions/bounty/<int:bounty_id>/interest/new/', dashboard.views.new_interest, name='express-interest'),
path('actions/bounty/<int:bounty_id>/interest/remove/', dashboard.views.remove_interest, name='remove-interest'),
path(
'actions/bounty/<int:bounty_id>/interest/<int:profile_id>/uninterested/',
'actions/bounty/<int:bounty_id>/interest/<int:profile_id>/uninterested/<int:slash>/',
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we drop the additional var in this URL?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, code updated!

@mbeacom mbeacom changed the title add 'stop work & slash' button WIP: add 'stop work & slash' button Jun 7, 2018
@@ -220,13 +230,21 @@ var mutate_interest = function(bounty_pk, direction, data) {
};


var uninterested = function(bounty_pk, profileId) {
var uninterested = function(bounty_pk, profileId, slash) {
var data = {}

Choose a reason for hiding this comment

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

Missing semicolon. (semi)

@owocki
Copy link
Contributor

owocki commented Jun 8, 2018

@owocki Are we married to "slash" here? Thoughts on alter_rep / ding_rep or something like that? After discussing it with @SaptakS, slash seems fairly misleading, since we use slash in multiple places to represent the presence of / and might prove difficult to maintain or approach by community members that are unfamiliar with the terminology.

Note: I understand the hunter wrote this to the spec, so I wouldn't expect him to modify this. Just something we might want to consider before we push it live.

I just used slash bc its what Vlad Zamfir uses as a verb when he talks about Proof of Stake. I'm not married to it; and I don't feel strongly in either direction (as this is an admin only functionality)

@owocki owocki merged commit 1d5af4a into gitcoinco:master Jun 12, 2018
@ghost ghost removed the in progress label Jun 12, 2018
@mbeacom mbeacom changed the title WIP: add 'stop work & slash' button add 'stop work & slash' button Jun 13, 2018
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.

Admin: Stop Work + Stop Work & Slash
5 participants