#2251 Different fixes around the private repo feature
Merged by pingou. Opened by pingou.
private_repo_fixes  into  master

Download 2251.patch
no initial comment

Pretty please pagure-ci rebuild

2 new commits added

  • Make private projects created via the API are namespaced
  • Add unit-tests checking private projects created in the UI are namespaced

rebased

9 new commits added

  • Make private projects created via the API are namespaced
  • Add unit-tests checking private projects created in the UI are namespaced
  • Add unit-tests checking that we cannot do public -> private
  • Set the false_values for the private field in the html form
  • Make the get_csrf method be in the Modeltests class so all can inherit it
  • Fix up Namespace private project
  • Allow private project to be made public but not the other way around
  • Namespace private project when they are created
  • Add something in the UI indicating that the project is private

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.

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.

@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.

I don't really see how this would affect a "redirect".

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

  • Add a custom validator to set the namespace when the project to create is private
  • Fix typo/grammar in the comment
  • No need to conditional the checked in the checkbox
  • Consolidate the list of all false values for the form in one place

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:

@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! :)

rebased

Pull-Request has been merged by pingou

Metadata