Pretty please pagure-ci rebuild
2 new commits added
rebased
9 new commits added
You seem to use this tuple a lot. Consider making this a member variable on PagureForm. That way if you need to change it, it's only in one location.
PagureForm
The documentation seems to expect a sequence of strings: http://wtforms.readthedocs.io/en/latest/fields.html#wtforms.fields.BooleanField
It doesn't seem to matter but it's possible the boolean and integer in the tuple don't take effect.
So you're only able to make a repo public, but not private once it's created?
If the above comment is true, you don't need the if statement on the line below anymore.
Typo, it should be something like: Although it's not needed for the new_project function, it fixes the redirect afterward
Optional: You could add a custom validator to WTForms to make sure that the value of "namespace" is the user's username when the repo is private. That way you don't have to overwrite the namespace.
I don't really see how this would affect a "redirect".
You could just disable CSRF protection in testing instead.
To me it's implied that search_projects without any parameters would return all projects including private ones. I suppose it doesn't change anything except code readability.
search_projects
@pingou I'm done reviewing. Just some minor comments.
Apart from the changes suggested by @mprahl I don't see much improvement but again I just had a shallow look on the PR i will test it once and let you know. :smile:
I'll make a variable for it indeed. I'll keep all the options as is for now, it may be useless but doesn't seem to do any harm either :)
That is correct
Good catch! :)
I'll adjust the comment using your suggestion.
It does because when the 'private' checkbox is checked, the namespace input is disabled so nothing will get submitted, making the namespace variable empty. So we set it here so that the redirect works at the end :)
Would make testing the csrf mechanism harder no? :)
When we added the private repo feature this would have changed the API of search_projects with the risk of missing one and exposing private projects, so we made that decision.
4 new commits added
Took me a little time but got it done :)
This is up to you, but instead of changing the data, you could raise a ValidationError from WTForms. That way the user is notified that they are trying to circumvent the UI restrictions rather than fixing it for them. :smile:
ValidationError
@pingou, one minor recommendation. +1 after you decide which route you want to go with this.
LGTM I tried out the patch
The logic in pagure.lib is already fixing this for them, so I'll keep the behaviour consistent here.
Thanks for the idea though, I gave it a try before realizing I wasn't being consistent.
Thanks for the review folks! :)
Pull-Request has been merged by pingou