PR#82: Add bugzilla override functionality - pagure-dist-git

pagure-dist-git

#82 Add bugzilla override functionality

Merged 4 years ago by pingou. Opened 4 years ago by karsten.

karsten/pagure-dist-git bzoverrides into master

Add an example config file to enable the dist-git plugin

Karsten Hopp • 4 years ago

93e96d3

Add bugzillla override functions and tests

Karsten Hopp • 4 years ago

8f7bef7

README.rst

file modified

+6 -1

		`@@ -28,6 +28,9 @@`
		`refs.`
		- ``SIG_PREFIXES``: List of prefixes for SIG refs.

		`+ To enable this plugin, you need to either point the PAGURE_PLUGIN environment`
		`+ variable at the pagure_distgit_config file or use the --plugin parameter of`
		`+ the runserver.py script.`

		`Example configurations`
		`======================`
		`@@ -73,8 +76,10 @@`
		`modify your PYTHONPATH to find them. Run with::`

		`$ PYTHONPATH=.:/path/to/pagure/checkout nosetests dist_git_auth_tests.py`
		`+ or`
		`+ $ PYTHONPATH=.:/path/to/pagure/checkout nosetests bugzilla-override-tests.py`

		`You can use our requirements-testing.txt to install testing dependencies with pip:`
		`$ pip install -r /path/to/pagure/checkout/requirements.txt`
		`$ pip install -r /path/to/pagure/checkout/requirements-testing.txt`
		`- $ pip install -r requirements-testing.txt`
		`\ No newline at end of file`
		`+ $ pip install -r requirements-testing.txt`

bugzilla-override-tests.py

file added

+198

		`@@ -0,0 +1,198 @@`
		`+ from __future__ import print_function`
		`+`
		`+ import os`
		`+ import json`
		`+`
		`+ # These are the tests from the pagure/ git repo.`
		`+ # Run with:`
		`+ # PYTHONPATH=.:/path/to/pagure/checkout nosetests dist_git_auth_tests.py`
		`+ import tests`
		`+`
		`+ from pagure_distgit import plugin`
		`+`
		`+`
		`+ def setUp():`
		`+ tests.setUp()`
		`+`
		`+`
		`+ def tearDown():`
		`+ tests.tearDown()`
		`+`
		`+`
		`+ class PagureFlaskApiProjectBZOverrideTests(tests.Modeltests):`
		`+ def setUp(self):`
		`+ """ Set up the environnment, ran before every tests. """`
		`+ super(PagureFlaskApiProjectBZOverrideTests, self).setUp()`
		`+ self.session.flush()`
		`+ tests.create_projects(self.session)`
		`+ tests.create_projects_git(os.path.join(self.path, "repos"), bare=True)`
		`+ tests.create_tokens(self.session)`
		`+ tests.create_tokens_acl(self.session)`
		`+ self._app.register_blueprint(plugin.DISTGIT_NS)`
		`+`
		`+ def test_override_endpoint(self):`
		`+ """Test bugzilla overrides. """`
		`+`
		`+ output = self.app.get("/api/0/somenamespace/test3")`
		`+ self.assertEqual(output.status_code, 200)`
		`+ output = self.app.get("/_dg/bzoverrides/somenamespace/test3")`
		`+ self.assertEqual(output.status_code, 200)`
		`+ data = json.loads(output.get_data(as_text=True))`
		`+ expected_result = {"epel_assignee": "pingou", "fedora_assignee": "pingou"}`
		`+ data = json.loads(output.get_data(as_text=True))`
		`+ self.assertDictEqual(data, expected_result)`
		`+`
		`+ headers = {"Authorization": "token foo_token"}`
		`+ datainput = {`
		`+ "epel_assignee": "foo",`
		`+ "fedora_assignee": "pingou",`
		`+ }`
		`+ output = self.app.post(`
		`+ "/_dg/bzoverrides/somenamespace/test3", data=datainput, headers=headers`
		`+ )`
		`+ # invalid token`
		`+ self.assertEqual(output.status_code, 401)`
		`+`
		`+ headers = {"Authorization": "token BBBZZZOOO"}`
		`+ output = self.app.post(`
		`+ "/_dg/bzoverrides/somenamespace/test3", data=datainput, headers=headers`
		`+ )`
		`+ self.assertEqual(output.status_code, 401)`
		`+`
		`+ output = self.app.get("/_dg/bzoverrides/somenamespace/test3")`
		`+ self.assertEqual(output.status_code, 200)`
		`+ data = json.loads(output.get_data(as_text=True))`
		`+ self.assertDictEqual(data, expected_result)`
		`+`
		`+ # Change assignees with a valid token`
		`+ headers = {"Authorization": "token aaabbbcccddd"}`
		`+ output = self.app.post(`
		`+ "/_dg/bzoverrides/somenamespace/test3", data=datainput, headers=headers`
		`+ )`
		`+ self.assertEqual(output.status_code, 200)`
		`+ expected_result = {"epel_assignee": "foo", "fedora_assignee": "pingou"}`
		`+ output = self.app.get("/_dg/bzoverrides/somenamespace/test3")`
		`+ data = json.loads(output.get_data(as_text=True))`
		`+ self.assertDictEqual(data, expected_result)`
		`+`
		`+ # set both assignees to foo`
		`+ datainput = {"epel_assignee": "foo", "fedora_assignee": "foo"}`
		`+ output = self.app.post(`
		`+ "/_dg/bzoverrides/somenamespace/test3", data=datainput, headers=headers`
		`+ )`
		`+ self.assertEqual(output.status_code, 200)`
		`+ output = self.app.get("/_dg/bzoverrides/somenamespace/test3")`
		`+ data = json.loads(output.get_data(as_text=True))`
		`+ self.assertDictEqual(data, datainput)`
		`+`
		`+ # Change both assignees back to default`
		`+ datainput = {"epel_assignee": "", "fedora_assignee": None}`
		`+ expected_result = {"epel_assignee": "pingou", "fedora_assignee": "pingou"}`
		`+ output = self.app.post(`
		`+ "/_dg/bzoverrides/somenamespace/test3", data=datainput, headers=headers`
		`+ )`
		`+ self.assertEqual(output.status_code, 200)`
		`+ output = self.app.get("/_dg/bzoverrides/somenamespace/test3")`
		`+ data = json.loads(output.get_data(as_text=True))`
		`+ self.assertDictEqual(data, expected_result)`
		`+`
		`+ # Change only one assignee, then change assignee back to default`
		`+ expected_result = {"epel_assignee": "pingou", "fedora_assignee": "pingou"}`
		`+ datainput = {"epel_assignee": "foo", "fedora_assignee": "pingou"}`
		`+ output = self.app.post(`
		`+ "/_dg/bzoverrides/somenamespace/test3", data=datainput, headers=headers`
		`+ )`
		`+ self.assertEqual(output.status_code, 200)`
		`+ datainput = {"epel_assignee": "", "fedora_assignee": "pingou"}`
		`+ output = self.app.post(`
		`+ "/_dg/bzoverrides/somenamespace/test3", data=datainput, headers=headers`
		`+ )`
		`+ self.assertEqual(output.status_code, 200)`
		`+ output = self.app.get("/_dg/bzoverrides/somenamespace/test3")`
		`+ data = json.loads(output.get_data(as_text=True))`
		`+ self.assertDictEqual(data, expected_result)`
		`+`
		`+ user = tests.FakeUser(username="pingou")`
		`+ with tests.user_set(self.app.application, user):`
		`+ # change one assignee`
		`+ expected_result = {"epel_assignee": "foo", "fedora_assignee": "pingou"}`
		`+ datainput = {"epel_assignee": "foo", "fedora_assignee": "pingou"}`
		`+ output = self.app.post(`
		`+ "/_dg/bzoverrides/somenamespace/test3", data=datainput`
		`+ )`
		`+ self.assertEqual(output.status_code, 200)`
		`+ data = json.loads(output.get_data(as_text=True))`
		`+ self.assertDictEqual(data, expected_result)`
		`+`
		`+ # change one assignee`
		`+ expected_result = {"epel_assignee": "foo", "fedora_assignee": "foo"}`
		`+ datainput = {"epel_assignee": "foo","fedora_assignee": "foo"}`
		`+ output = self.app.post(`
		`+ "/_dg/bzoverrides/somenamespace/test3", data=datainput`
		`+ )`
		`+ self.assertEqual(output.status_code, 200)`
		`+ data = json.loads(output.get_data(as_text=True))`
		`+ self.assertDictEqual(data, expected_result)`
		`+`
		`+ # reset assignees`
		`+ expected_result = {"epel_assignee": "pingou", "fedora_assignee": "pingou"}`
		`+ datainput = {"epel_assignee": None, "fedora_assignee": ""}`
		`+ output = self.app.post(`
		`+ "/_dg/bzoverrides/somenamespace/test3", data=datainput`
		`+ )`
		`+ self.assertEqual(output.status_code, 200)`
		`+ data = json.loads(output.get_data(as_text=True))`
		`+ self.assertDictEqual(data, expected_result)`
		`+`
		`+ # change both assignees to foo, then change only one back`
		`+ # As the other is considered empty, both should be back to pingou`
		`+ expected_result = {"epel_assignee": "foo", "fedora_assignee": "foo"}`
		`+ datainput = {"epel_assignee": "foo","fedora_assignee": "foo"}`
		`+ output = self.app.post(`
		`+ "/_dg/bzoverrides/somenamespace/test3", data=datainput`
		`+ )`
		`+ self.assertEqual(output.status_code, 200)`
		`+ data = json.loads(output.get_data(as_text=True))`
		`+ self.assertDictEqual(data, expected_result)`
		`+ expected_result = {"epel_assignee": "pingou", "fedora_assignee": "pingou"}`
		`+ datainput = {"epel_assignee": "pingou"}`
		`+ output = self.app.post(`
		`+ "/_dg/bzoverrides/somenamespace/test3", data=datainput`
		`+ )`
		`+ self.assertEqual(output.status_code, 200)`
		`+ data = json.loads(output.get_data(as_text=True))`
		`+ self.assertDictEqual(data, expected_result)`
		`+`
		`+ # Invalid datainput resets to default`
		`+ datainput = {"epel_assignee": "foo","fedora_assignee": "foo"}`
		`+ output = self.app.post(`
		`+ "/_dg/bzoverrides/somenamespace/test3", data=datainput`
		`+ )`
		`+ self.assertEqual(output.status_code, 200)`
		`+ datainput = {} # This is invalid input`
pingou commented 4 years ago I disagree, it is entirely valid
		`+ output = self.app.post(`
		`+ "/_dg/bzoverrides/somenamespace/test3", data=datainput`
		`+ )`
		`+ self.assertEqual(output.status_code, 200)`
		`+ data = json.loads(output.get_data(as_text=True))`
		`+ self.assertDictEqual(data, expected_result)`
		`+`
		`+ datainput = {"epel_assignee": "foo","fedora_assignee": "foo"}`
		`+ output = self.app.post(`
		`+ "/_dg/bzoverrides/somenamespace/test3", data=datainput`
		`+ )`
		`+ self.assertEqual(output.status_code, 200)`
		`+ datainput = None # Invalid input`
		`+ output = self.app.post(`
		`+ "/_dg/bzoverrides/somenamespace/test3", data=datainput`
		`+ )`
		`+ data = json.loads(output.get_data(as_text=True))`
		`+ self.assertEqual(output.status_code, 200)`
		`+`
		`+ user = tests.FakeUser(username="foo")`
		`+ with tests.user_set(self.app.application, user):`
		`+ expected_result = {"epel_assignee": "pingou", "fedora_assignee": "pingou"}`
		`+ output = self.app.post(`
		`+ "/_dg/bzoverrides/somenamespace/test3", data=expected_result`
		`+ )`
		`+ self.assertEqual(output.status_code, 401)`

createdb.py

file modified

+2 -2

		`@@ -29,7 +29,7 @@`
		`os.environ["PAGURE_CONFIG"] = config`

		`import pagure.config`
		`- from pagure_distgit.model import BASE, PagureAnitya`
		`+ from pagure_distgit.model import BASE, PagureAnitya, PagureBZOverride`

		`db_url = pagure.config.config.get("DB_URL")`
		`if db_url.startswith("postgres"):`
		`@@ -37,4 +37,4 @@`
		`else:`
		`engine = create_engine(db_url, echo=True)`

		`- BASE.metadata.create_all(engine, tables=[PagureAnitya.__table__])`
		`+ BASE.metadata.create_all(engine, tables=[PagureAnitya.__table__, PagureBZOverride.__table__])`

pagure_distgit/forms.py

file modified

+15

		`@@ -5,6 +5,7 @@`

		`Authors:`
		`Pierre-Yves Chibon <pingou@pingoured.fr>`
		`+ Karsten Hopp <karsten@redhat.com>`

		`"""`

		`@@ -27,3 +28,17 @@`
		`("monitoring-with-scratch", "monitoring-with-scratch"),`
		`],`
		`)`
		`+`
		`+`
		`+ class BZOverrideForm(pagure.forms.PagureForm):`
		`+ """ Form to configure the Fedora/EPEL maintainer for a project. """`
		`+`
		`+ fedora_assignee = wtforms.StringField(`
		`+ "Maintainer name",`
		`+ [ wtforms.validators.optional(strip_whitespace=True) ],`
		`+ )`
		`+ epel_assignee = wtforms.StringField(`
		`+ "EPEL Maintainer name",`
		`+ [ wtforms.validators.optional(strip_whitespace=True) ],`
		`+ )`
		`+`

pagure_distgit/model.py

file modified

+36

		`@@ -57,3 +57,39 @@`
		`uselist=False,`
		`),`
		`)`
		`+`
		`+`
		`+ class PagureBZOverride(BASE):`
		`+ """ Stores information about default assignees (Fedora vs. EPEL)`
		`+`
		`+ Table -- pagure_bzoverride`
		`+ """`
		`+`
		`+ __tablename__ = "pagure_bzoverride"`
		`+`
		`+ id = sa.Column(sa.Integer, primary_key=True)`
		`+ project_id = sa.Column(`
		`+ sa.Integer,`
		`+ sa.ForeignKey("projects.id", onupdate="CASCADE", ondelete="CASCADE"),`
		`+ nullable=False,`
		`+ unique=True,`
		`+ index=True,`
		`+ )`
		`+`
		`+ epel_assignee = sa.Column(`
		`+ sa.String(255), unique=False`
		`+ )`
		`+ fedora_assignee = sa.Column(`
		`+ sa.String(255), unique=False`
		`+ )`
		`+`
		`+ project = relation(`
		`+ "Project",`
		`+ remote_side=[Project.id],`
		`+ backref=backref(`
		`+ "bzoverride",`
		`+ cascade="delete, delete-orphan",`
		`+ single_parent=True,`
		`+ uselist=False,`
		`+ ),`
		`+ )`

pagure_distgit/plugin.py

file modified

+72

		`@@ -5,6 +5,7 @@`

		`Authors:`
		`Pierre-Yves Chibon <pingou@pingoured.fr>`
		`+ Karsten Hopp <karsten@redhat.com>`

		`"""`

		`@@ -374,3 +375,74 @@`
		`return html_output`
		`else:`
		`return flask.jsonify({"releases": releases, "updates": updates})`
		`+`
		`+`
		`+ @DISTGIT_NS.route("/bzoverrides/<namespace>/<repo>", methods=["GET"])`
		`+ @api_method`
		`+ def bzoverride_get_endpoint(repo, namespace):`
		`+ """ Returns the current default assignee(s) of this package.`
		`+ Defaults to the repo user if unset.`
		`+ """`
		`+ repo = _get_repo(repo, namespace=namespace)`
		`+ output = {"fedora_assignee": repo.user.username,`
		`+ "epel_assignee": repo.user.username }`
		`+ if repo.bzoverride:`
		`+ if repo.bzoverride.fedora_assignee:`
		`+ output["fedora_assignee"] = repo.bzoverride.fedora_assignee`
		`+ if repo.bzoverride.epel_assignee:`
		`+ output["epel_assignee"] = repo.bzoverride.epel_assignee`
		`+ return flask.jsonify(output)`
		`+`
		`+`
		`+ @DISTGIT_NS.route("/bzoverrides/<namespace>/<repo>", methods=["POST"])`
		`+ @api_method`
		`+ @api_login_required(acls=["modify_project"])`
		`+ def bzoverride_patch_endpoint(repo, namespace):`
		`+ """ Updates the default assignees of this package.`
		`+ """`
		`+`
		`+ repo = _get_repo(repo, namespace=namespace)`
		`+`
		`+ is_site_admin = pagure.utils.is_admin()`
		`+ # Only allow the main admin and Pagure site admins to modify projects'`
		`+ # monitoring, even if the user has the right ACLs on their token`
		`+ if (`
		`+ flask.g.fas_user.username != repo.user.username`
		`+ and not is_site_admin`
		`+ ):`
		`+ raise pagure.exceptions.APIError(`
		`+ 401, error_code=APIERROR.EMODIFYPROJECTNOTALLOWED`
		`+ )`
		`+`
		`+ form = pagure_distgit.forms.BZOverrideForm(csrf_enabled=False)`
		`+ if form.validate_on_submit():`
		`+ if form.fedora_assignee.data:`
		`+ fedora_assignee = form.fedora_assignee.data.strip()`
		`+ else:`
		`+ fedora_assignee = None`
		`+ if form.epel_assignee.data:`
		`+ epel_assignee = form.epel_assignee.data.strip()`
		`+ else:`
		`+ epel_assignee = None`
		`+ try:`
		`+ if repo.bzoverride:`
		`+ repo.bzoverride.fedora_assignee = fedora_assignee`
		`+ repo.bzoverride.epel_assignee = epel_assignee`
		`+ flask.g.session.add(repo)`
		`+ else:`
		`+ mapping = model.PagureBZOverride(`
		`+ project_id=repo.id, epel_assignee=epel_assignee,`
		`+ fedora_assignee=fedora_assignee`
		`+ )`
		`+ flask.g.session.add(mapping)`
		`+ flask.g.session.commit()`
		`+ except SQLAlchemyError as err: # pragma: no cover`
		`+ flask.g.session.rollback()`
		`+ _log.exception(err)`
		`+ raise pagure.exceptions.APIError(400, error_code=APIERROR.EDBERROR)`
		`+ else:`
		`+ raise pagure.exceptions.APIError(`
		`+ 400, error_code=APIERROR.EINVALIDREQ, errors=form.errors`
		`+ )`
		`+`
		`+ return bzoverride_get_endpoint(repo=repo.name, namespace=namespace)`

pagure_distgit_config

file added

+11

		`@@ -0,0 +1,11 @@`
		`+ # This file can be used to enable the dist-git plugin by pointing`
		`+ # the environment variable PAGURE_PLUGIN at it or by using`
		`+ # the --plugin parameter of the runserver.py script`
		`+`
		`+ import os`
		`+ import sys`
		`+`
		`+ dir_path = os.path.dirname(os.path.realpath(__file__))`
		`+ from pagure_distgit import plugin`
		`+`
		`+ PLUGINS = [plugin.DISTGIT_NS]`

karsten commented 4 years ago

I had to disable the access checks as for some reason the line
flask.g.fas_user.username not in admins

results in a traceback:

  File "/home/vagrant/.virtualenvs/python3-pagure/lib/python3.7/site-packages/flask/app.py", line 1935, in dispatch_request
    return self.view_functions[rule.endpoint](**req.view_args)
  File "/home/vagrant/devel/pagure/api/__init__.py", line 223, in wrapper
    result = function(*args, **kwargs)
  File "/home/vagrant/plugins/pagure-dist-git/pagure_distgit/plugin.py", line 390, in bzoverride_patch_endpoint
    flask.g.fas_user.username not in admins
  File "/home/vagrant/.virtualenvs/python3-pagure/lib/python3.7/site-packages/werkzeug/local.py", line 348, in __getattr__
    return getattr(self._get_current_object(), name)
AttributeError: '_AppCtxGlobals' object has no attribute 'fas_user'

pingou commented 4 years ago

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

1 new commit added

add api_method, thanks to jlanda

4 years ago

2 new commits added

add missing imports, fix typo
enable api_login_required (pingou)

4 years ago

karsten commented 4 years ago

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

pingou commented 4 years ago

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.

pingou commented on line 29 of pagure_distgit/forms.py 4 years ago

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

pingou commented on line 21 of pagure_distgit/plugin.py 4 years ago

flask.g.repo isn't working?

pingou commented on line 39 of pagure_distgit/forms.py 4 years ago

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.

karsten commented 4 years ago

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 ?

1 new commit added

better solution from Pierre with 2 StringFields, various fixes

4 years ago

pingou commented 4 years ago

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?

1 new commit added

enable permission checks

4 years ago

pingou commented on line 45 of pagure_distgit/plugin.py 4 years ago

I see where this comes from ;-)

2 new commits added

use underscore to make it easier to handle in javascript
remove old wtform

4 years ago

pingou commented 4 years ago

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

rebased onto 863f222a4e769ff4083b17a3c73d3657767fbf09

4 years ago

1 new commit added

remove conflict markers

4 years ago

pingou commented on line 27 of bugzilla-override-tests.py 4 years ago

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().

pingou commented on line 47 of bugzilla-override-tests.py 4 years ago

Note that this isn't flake8/black valid.

pingou commented on line 19 of dist_git_auth_tests.py 4 years ago

As above, this section is not needed

pingou commented on line 21 of pagure_distgit/plugin.py 4 years ago

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

pingou commented on line 23 of pagure_distgit/forms.py 4 years ago

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.

karsten commented 4 years ago

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

pingou commented 4 years ago

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?

3 new commits added

flake8 and black fixes
more tests
drop routes without namespace

4 years ago

karsten commented 4 years ago

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

2 new commits added

Strip the assignee string, redirect to info page
Remove UserValidator

4 years ago

karsten commented 4 years ago

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

pingou commented on line 48 of bugzilla-override-tests.py 4 years ago

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

pingou commented on line 64 of bugzilla-override-tests.py 4 years ago

Why is pagure.cli here?

pingou commented 4 years ago

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 :(

pingou commented on line 713 of dist_git_auth_tests.py 4 years ago

Why is pagure.cli involved here?

karsten commented 4 years ago

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.

1 new commit added

Strip whitespace from input

4 years ago

karsten commented 4 years ago

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.

1 new commit added

various fixes from the review

4 years ago

pingou commented 4 years ago

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.

pingou commented on line 29 of pagure_distgit/plugin.py 4 years ago

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

pingou commented on line 37 of pagure_distgit/plugin.py 4 years ago

namespace cannot be None

pingou commented on line 55 of pagure_distgit/plugin.py 4 years ago

Same as above

pingou commented on line 35 of pagure_distgit/plugin.py 4 years ago

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

pingou commented on line 41 of pagure_distgit/plugin.py 4 years ago

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

pingou commented on line 710 of dist_git_auth_tests.py 4 years ago

I've got the tests pass locally without this line

pingou commented on line 13 of pagure_distgit/forms.py 4 years ago

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

pingou commented on line 2 of pagure_distgit_config 4 years ago

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

1 new commit added

Added more fixes from the review

4 years ago

karsten commented 4 years ago

The latest commit has fixes for all the mentioned issues

pingou commented on line 763 of dist_git_auth_tests.py 4 years ago

commented-out a line that was already commented out?

pingou commented 4 years ago

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

rebased onto 688374120b6d659bcbffe018c091d4454e71f56a

4 years ago

karsten commented 4 years ago

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

pingou commented 4 years ago

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

pingou commented on line 3 of pagure_distgit_config 4 years ago

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

pingou commented on line 45 of dist_git_auth_tests.py 4 years ago

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

1 new commit added

Move override tests to its own file

4 years ago

pingou commented on line 34 of bugzilla-override-tests.py 4 years ago

not needed

pingou commented on line 35 of bugzilla-override-tests.py 4 years ago

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

pingou commented on line 6 of dist_git_auth_tests.py 4 years ago

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.

rebased onto c831745063f20167b30e83d877ed2383ff63d55a

4 years ago

2 new commits added

A couple smaller fixes to address comments in the review
Document config file usage in the README

4 years ago

karsten commented 4 years ago

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.

pingou commented 4 years ago

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

jlanda commented on line 40 of bugzilla-override-tests.py 4 years ago

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

jlanda commented 4 years ago

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

1 new commit added

Add tests pith pagure session instead of tokens

4 years ago

karsten commented 4 years ago

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.

jlanda commented 4 years ago

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

wfm

pingou commented on line 77 of pagure_distgit/plugin.py 4 years ago

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

pingou commented 4 years ago

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

rebased onto f6aa2b6cc5126af15aed6dec61a19be6743a8d00

4 years ago

karsten commented 4 years ago

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

pingou commented on line 2 of scripts/migrate.py 4 years ago

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.

2 new commits added

Add an example config file to enable the dist-git plugin
Add bugzillla override functions and tests

4 years ago

karsten commented 4 years ago

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

2 new commits added

Add an example config file to enable the dist-git plugin
Add bugzillla override functions and tests

4 years ago

pingou commented on line 83 of bugzilla-override-tests.py 4 years ago

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

pingou commented on line 92 of bugzilla-override-tests.py 4 years ago

same as above

pingou commented on line 41 of bugzilla-override-tests.py 4 years ago

should be only one empty row

pingou commented on line 31 of bugzilla-override-tests.py 4 years ago

iirc, there should be an empty row here

pingou commented on line 80 of pagure_distgit/plugin.py 4 years ago

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

karsten commented 4 years ago

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.

jlanda commented 4 years ago

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

2 new commits added

Add an example config file to enable the dist-git plugin
Add bugzillla override functions and tests

4 years ago

karsten commented 4 years ago

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

pingou commented 4 years ago

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

jlanda commented 4 years ago

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

pingou commented 4 years ago

Good catch, I had checked the overall diff :(

jlanda commented on line 29 of pagure_distgit/model.py 4 years ago

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

pingou commented 4 years ago

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

pingou commented on line 29 of pagure_distgit/model.py 4 years ago

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

rebased onto dcf922555fce0062892f5cedaca53f0c046400fc

4 years ago

rebased onto bd441f6f3fee398376a4e34f089e17ce04250500

4 years ago

karsten commented 4 years ago

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

pingou commented 4 years ago

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

rebased onto 084a1edb7a04d8c2d049c282abc8784f8a1e7c71

4 years ago

karsten commented 4 years ago

fixed

Metadata Update from @pingou:
- Request assigned

4 years ago

pingou commented on line 99 of bugzilla-override-tests.py 4 years ago

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

pingou commented on line 99 of bugzilla-override-tests.py 4 years ago

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

rebased onto 9ff007158b535eed27692591c448a92843c6919d

4 years ago

rebased onto 3656066ee166ce023fb8003708f9fdff346f9d5a

4 years ago

karsten commented 4 years ago

I've added a couple more tests as requested

pingou commented on line 141 of bugzilla-override-tests.py 4 years ago

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

rebased onto 8f7bef7

4 years ago

karsten commented 4 years ago

I have now and updated the tests

pingou commented on line 173 of bugzilla-override-tests.py 4 years ago

I disagree, it is entirely valid

jlanda commented 4 years ago

I disagree, it is entirely valid

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

pingou commented 4 years ago

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.

Pull-Request has been merged by pingou

4 years ago