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

Do not build projects from banned users #5096

Merged
merged 1 commit into from Jan 15, 2019

Conversation

@humitos
Copy link
Member

@humitos humitos commented Jan 12, 2019

If any owner of the project is banned, we skip building this project.

If any owner of the project is banned, we skip building this project.
@humitos humitos requested a review from Jan 12, 2019
Copy link
Member

@ericholscher ericholscher left a comment

Seems reasonable. I do worry about false positives here, where a valid user is banned, perhaps because a spammer added them to a spam project. Having no feedback on why your project isn't building is bad UX, but so is telling the spammer that we know they are a spammer. :/

@humitos
Copy link
Member Author

@humitos humitos commented Jan 14, 2019

I do worry about false positives here, where a valid user is banned, perhaps because a spammer added them to a spam project

When we "ban the owner" of a project with multiple users, an exception is risen because of this reason.

https://github.com/rtfd/readthedocs.org/blob/b07f4550345b90e07fd2504b984c3361a05a97c6/readthedocs/projects/admin.py#L165-L167

The other case is true, but it's unlikely: "a valid user add a spammer user into a valid project".

Having no feedback on why your project isn't building is bad UX, but so is telling the spammer that we know they are a spammer. :/

I do see two different ways of giving feedback here (not for this PR though):

  1. Add a permanent message that appears while is_active == True
  2. Create the Build object with a pre-filled error without triggering the Celery task

(I remember that we used to have the message hardcoded in the template for project.skip == True but we removed it because we said that was a bad pattern and we wanted to use our notification system)

@humitos humitos self-assigned this Jan 15, 2019
Copy link
Member

@ericholscher ericholscher left a comment

Cool. Let's ship this and we can worry about the side effects later, I think. Hopefully we shouldn't be banning users accidentally :)

@ericholscher ericholscher merged commit 7aa57bb into master Jan 15, 2019
2 checks passed
@delete-merged-branch delete-merged-branch bot deleted the humitos/skip-build-on-banned-users branch Jan 15, 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
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants