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

Prevent yanking of popular gem version #1377

Closed
wants to merge 1 commit into from

Conversation

shlok007
Copy link
Contributor

@shlok007 shlok007 commented Aug 7, 2016

Refers to issue #1226 .

Gem yanks will only be allowed if the version has less than 15,000 downloads. This will allow relatively unused gems to be yanked freely, while popular gems will have a shorter period of time to realize they have a legit reason to yank.

@@ -171,6 +171,10 @@ def platformed?

delegate :reorder_versions, to: :rubygem

def can_yank
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about adding a ? to this method? So it becomes can_yank?? Or maybe yankable??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason I didn't use yankable is because it isn't an actual word by dictionary.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah ok :-) Can we in that case at the ??

@sonalkr132
Copy link
Member

NOTE: This PR is an alternate solution to PR #1371

Thank you for your PR @shlok007 💙
Instead of adding a conditional in controller, couldn't this be implemented as a validation on deletion model?

@shlok007
Copy link
Contributor Author

shlok007 commented Aug 7, 2016

Thank you. 😃
What you suggested might be a better way but still I would have to modify the controller and add the condition to display the appropriate message after the yank fails. This is the current text which is rendered on a yank failure which won't be appropriate in this situation.

@sonalkr132
Copy link
Member

This is the current text which is rendered on a yank failure which won't be appropriate in this situation.

IMHO, the current text is not appropriate for failure of @deletion.save as it can return false for reasons other than version already being deleted. I believe using conventional @deletion.errors should be fine unless I am missing something here.

cc: @dwradcliffe

if @deletion.save
StatsD.increment 'yank.success'
render text: "Successfully deleted gem: #{@version.to_title}"
if @version.can_yank?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jvanbaarsen Done. :)

@indirect
Copy link
Member

I agree with @sonalkr132's suggestion, and I think the error message should come from a validation.

I would also like this to show a clear error message with wording like "Gem versions cannot be yanked after they have been downloaded over 15,000 times".

@sonalkr132, if you have time now, I would be happy to merge a PR based on this that makes those changes.

@sonalkr132
Copy link
Member

if you have time now, I would be happy to merge a PR based on this that makes those changes.

@shlok007 wanted to work on this. I am sure he can make requested change 👌

@indirect
Copy link
Member

👍💯

@shlok007
Copy link
Contributor Author

I have made a separate PR covering those changes. #1396 . Closing this.

@shlok007 shlok007 closed this Aug 28, 2016
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.

None yet

4 participants