The login required is turned off, so I guess it makes sense that pagure complains that the user is not logged in.

dang, that's what you get if you don't check the sources and just guess what something does. My guess was that the api_login_required just checks if I'm logged in and not that it disables logins altogether. Thanks

dang, that's what you get if you don't check the sources and just guess what something does. My guess was that the api_login_required just checks if I'm logged in and not that it disables logins altogether. Thanks

IT does check if you're logged in and since it says required if you are not logged in, it boots you out if you aren't. So any endpoint having this decorator can assume that flask.g.fas_user is not None.

You used wtforms.validators below, let's be consistent and use it in these exceptions as well :)

flask.g.repo isn't working?

I'm curious why not simply have two StringField for fedora_assignee and epel_assignee?
Then if I send a POST request where epel_assignee is None it means I'm removing the overrides, thus resetting its value.

flask.g.repo isn't working?

Not unless there's some secret magic that I'm not aware off. I'm sending the repo with POST, how is flask supposed to know that I'm changing variables in arbitrary repositories if I don't use the repo string from POST to search for a repo with that name ?

how is flask supposed to know that I'm changing variables in arbitrary repositories if I don't use the repo string from POST to search for a repo with that name ?

Via the variable set in the URLs?

I see where this comes from ;-)

Hm, quick question: what is the expected behavior for non-rpms namespaces (ie, modules, containers...) ?

There is simpler, you can just register the blueprint in the setUp(), no need to keep that configuration file here, no need to mess with the path, no need for the global setUp() and tearDown().

Note that this isn't flake8/black valid.

As above, this section is not needed

Do we have repo in dist-git that have no namespace?

This assumes the user will exists in pagure's DB which is not an assumption that we can make. We need to go check if the user exists in FAS.

no need for the global setUp() and tearDown().

I've moved tests.setUp() into the local class as otherwise nothing calls setUp and tearDown from tests.
Modeltests setup() calls setUp() from SimplePagureTest, but that one doesn't call the global setUp() in tests/init

no need for the global setUp() and tearDown().

I've moved tests.setUp() into the local class as otherwise nothing calls setUp and tearDown from tests.
Modeltests setup() calls setUp() from SimplePagureTest, but that one doesn't call the global setUp() in tests/init

Isn't that nosetests' job?

no need for the global setUp() and tearDown().
I've moved tests.setUp() into the local class as otherwise nothing calls setUp and tearDown from tests.
Modeltests setup() calls setUp() from SimplePagureTest, but that one doesn't call the global setUp() in tests/init

Isn't that nosetests' job?

Maybe, but then it isn't doing a good job. The db won't get set up unless I call tests.setUp() by myself

I've removed the user validation as we've discussed. It can be added again later when we can query FAS for users/groups.

create_projects() creates a namespaced project already, any reasons for not using it?

Why is pagure.cli here?

I appreciate that you run black on the sources, but it makes this review much harder. For each diff I now have to figure out if the logic changed or just the style :(

Why is pagure.cli involved here?

Why is pagure.cli here?

please ignore bugzilla-override-tests.py, I'll remove it. Everything in there is also in dist_git_auth_tests.py, it was just intended to shorten my test runs.

create_projects() creates a namespaced project already, any reasons for not using it?

somenamespace/test3 gets created to early, before I have a chance to register the blueprint for dist-git and thus is lacking the endpoints that I want to test.

super(PagureFlaskApiProjectBZOverrideTests, self).setUp() creates the namespaced project, but it also creates self._app which I need to register the blueprint.

somenamespace/test3 gets created to early, before I have a chance to register the blueprint for dist-git and thus is lacking the endpoints that I want to test.

Nope, the db and the blueprint are not related, so that must be something else :)

Also, I think I like your original idea to put these tests in a separate file. After all that file is for testing dist_git_auth.py not pagure_distgit.

While I see the point of this endpoint for testing, I don't think we want it merged.

namespace cannot be None

Same as above

Let's be consistent and put this one one line below, after the declaration of the route.

Let's use the same construct as you used below, it's clearer

I've got the tests pass locally without this line

This import doesn't seem to be used, so it looks like we can remove it.

This file may be an good addition, but then we should document what it is and how to use it

The latest commit has fixes for all the mentioned issues

commented-out a line that was already commented out?

There are still a lot of changes not really related to this change but it starts to look like something so let's start cleaning up the commit lists :)

I've cleaned up the changes, the commit list and the commit message.

Also, I think I like your original idea to put these tests in a separate file. After all that file is for testing dist_git_auth.py not pagure_distgit.

I think this comment still applies :)

If you want to include this file in this PR, then it should have its own commit.
Note that runserver.py in pagure has the following option: --plugins PLUGINS Configuration file for pagure plugin.

Also, it may be worth documenting this in the README, finding and reading this file is now the most discoverable :)

This is still not needed, using somenamespace/test3 works fine

not needed

user ad group? typo ad/and? (but I'm not seeing a group being added)

I'm not seeing this one being used.

Let's drop the changes to this file in this PR, we can do the clean up in the black one.

I've dropped the ALLOWED_PREFIX line, the 'ad group' line and the erroneous jason import. The config file now has its own commit and the README shows how it can be used.

Looks good, let's clean the up the commit list, I'll give it a final try and if all goes fine we'll merge this :)

pagure's test suite already sets two users: pingou and foo. You can use foo instead of override if you prefer :)

I miss a test with a pagure session against the api endpoint instead of api tokens. The ui part will use that, so it would be great to test both: api with tokens, api with flask session. There are a lot of examples on pagure's test suite (those with a with user blablabla

I've added two pagure session tests, one should successfully change the assignees, the other isn't allowed to change assignees.
I've also dropped the override user and use 'foo' instead.

I'm not a big fan of redirections on api endpoints result, but I can live with that.

wfm

This is an API endpoint, I don't think it should redirect to the UI/return HTML content

@karsten let's try to keep the commit list clean as otherwise we have to ask you to clean them instead of merging the PR once we're happy with it.

commit are down to one for the plugin config file and one for the rest of the changes

Let's keep this script out for now, if the diff keeps on adding/losing elements while we review it it's only making the review harder.
Also, since this is a one-of script I don't think we'll want to keep it here anyway.

ok, removed. I'll put it into a new repo to track it

I guess either it should not be commented or it should be removed.

same as above

should be only one empty row

iirc, there should be an empty row here

I don't understand this endpoint, it's not documented and it's still an API endpoint that return html

I don't understand this endpoint, it's not documented and it's still an API endpoint that return html

What I'm trying here is to add a function that I can use in pagure's repo_master_sidebar.html
A function that returns json is of no help.
I'm trying to figure out how to do this with javascript, so far without success.

What I'm trying here is to add a function that I can use in pagure's repo_master_sidebar.html
A function that returns json is of no help.
I'm trying to figure out how to do this with javascript, so far without success.

Anitya integration and orphaning take options use API endpoint's on dist-git + jquery ajax calls on src.fp.o theme to build their logic. Your implementation must be feasible to do with a similar approach:

On submit event on the button, send an ajax call to the POST endpoint. on the ajax call's success callback parse the response and modify DOM content accordingly

I've removed the endpoint with html response again and fixed the other issues in the test script that Pierre mentioned

@jlanda this PR is looking good to me.
Could you have a look at it as well to see if I've missed anything?

I don't like the commit log on dist_git_auth_tests.py

I'm fine with removing both imports if they're not nevessary, but first commit removes them to add an import json, the the next one removes rhe json import, and none of the commits is related with dist_git_auth_tests.py

I would prefer not touching that file on this pull request or having an exclusive commit that just remove them without the import json add+remove party

Good catch, I had checked the overall diff :(

Imho None for unassigned is semantically more correct than empty string for unassigned

https://pagure.io/fork/karsten/pagure-dist-git/c/e7528bdfa816fc0192cb744d208c5503e6a6d3ff
contains much more changes than the commit message hints for

dropping the nullable and the default are likely what we want here

dropped the nullable and the empty default string.

https://pagure.io/fork/karsten/pagure-dist-git/c/e7528bdfa816fc0192cb744d208c5503e6a6d3ff
contains much more changes than the commit message hints for

also fixed. 1faf602320b3717e851e6afd4c2d5d86d9495c9e is just for the config file, bd441f6f3fee398376a4e34f089e17ce04250500 has the rest of the changes

also fixed. 1faf602 is just for the config file, bd441f6 has the rest of the changes

The first changes in the README file are referring to a file that is added in the other commit :)

fixed

You're not testing anywhere the situation where the user only submits one of the two fields, or none of them, are you?

Or they are submitting the fields but with '' or None as value.

I've added a couple more tests as requested

Have you tried a request where data=None? or data={}?

I have now and updated the tests

I disagree, it is entirely valid

I disagree, it is entirely valid

+1

JSON does not have any difference between a non-existent key or a key with null value, so {} = { epel_assignee: null, fedora_ssignee: null} .

An input of {} should unassign both assignees

Ok, tests are passing and locally as well.

I'm going to merge this PR because this has been a long review already, but I'm going to open another PR refactoring the tests. The best practices are to have one test per function, instead of a bunch of tests in one function. In addition, we have two files with tests in this repo with this PR and they can't be run at the same time, that is a problem.

So I'm going to refactor some of the work done in this PR in another one.

Finally, I'll also run black in another PR, I'll note that while I want all the black changes to happen in one commit/PR, you could have run it on the files/changes you added.