#82 Add bugzilla override functionality
Merged 8 months ago by pingou. Opened 9 months ago by karsten.
karsten/pagure-dist-git bzoverrides  into  master

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

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

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)

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__])

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) ],

+     )

+ 

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,

+         ),

+     )

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)

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

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'

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
9 months ago

2 new commits added

  • add missing imports, fix typo
  • enable api_login_required (pingou)
9 months 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

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 ?

1 new commit added

  • better solution from Pierre with 2 StringFields, various fixes
9 months 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
9 months ago

I see where this comes from ;-)

2 new commits added

  • use underscore to make it easier to handle in javascript
  • remove old wtform
9 months ago

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

rebased onto 863f222

8 months ago

1 new commit added

  • remove conflict markers
8 months 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().

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?

3 new commits added

  • flake8 and black fixes
  • more tests
  • drop routes without namespace
8 months 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
8 months 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.

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

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.

1 new commit added

  • Strip whitespace from input
8 months 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
8 months 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.

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

namespace cannot be None

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

1 new commit added

  • Added more fixes from the review
8 months ago

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

rebased onto 6883741

8 months ago

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

1 new commit added

  • Move override tests to its own file
8 months ago

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.

rebased onto c831745

8 months ago

2 new commits added

  • A couple smaller fixes to address comments in the review
  • Document config file usage in the README
8 months 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.

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

1 new commit added

  • Add tests pith pagure session instead of tokens
8 months 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.

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.

rebased onto f6aa2b6

8 months ago

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.

2 new commits added

  • Add an example config file to enable the dist-git plugin
  • Add bugzillla override functions and tests
8 months 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
8 months ago

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

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

2 new commits added

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

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

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

rebased onto dcf9225

8 months ago

rebased onto bd441f6

8 months 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. 1faf602 is just for the config file, bd441f6 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 :)

rebased onto 084a1ed

8 months ago

Metadata Update from @pingou:
- Request assigned

8 months ago

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.

rebased onto 9ff0071

8 months ago

rebased onto 3656066

8 months ago

I've added a couple more tests as requested

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

rebased onto 8f7bef7

8 months ago

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.

Pull-Request has been merged by pingou

8 months ago