This is implementing https://pagure.io/pagure/issue/4605

Hm, if you're adding a new section you likely will have to do some changes in the template.

Otherwise, should they go under the projects section?

I don't see where are you using the tickets git repo on the tests

is this necessary?

Hm, if you're adding a new section you likely will have to do some changes in the template.
Otherwise, should they go under the projects section?

Hm, I'm not sure, this is a project settings, but issues also have they own section.
What do you think will be better, move them to projects section or modify the template?

I don't see where are you using the tickets git repo on the tests

I used the test_pagure_flask_api_issue_create.py as a template, this is probably some leftover. I will remove it.

is this necessary?

Probably not, I just wasn't sure if I can remove it.

Remove the tickets repo from the tests
Remove @api_login_optional decorator from api_view_plugins function
Update template with plugins.

Remove @api_login_optional decorator from api_view_plugins function

This may actually be needed, if the project is private, the user doing the GET request should be authenticated and if not, they should be refused the info

This method is just receiving every plugin and it's form fields, it's not tied to project.

This method is just receiving every plugin and it's form fields, it's not tied to project.

ok, cool then :)

Hm, the style is not failing on my vagrant machine and I don't know why it's failing in Jenkins.

I just missed the python3-nose in vagrant.

This is the error, not sure if I should fix it here:

Failure: ModuleNotFoundError (No module named 'mock') ... ERROR
======================================================================
ERROR: Failure: ModuleNotFoundError (No module named 'mock')
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/lib/python3.7/site-packages/nose/failure.py", line 39, in runTest
    raise self.exc_val.with_traceback(self.tb)
  File "/usr/lib/python3.7/site-packages/nose/loader.py", line 417, in loadTestsFromName
    addr.filename, addr.module)
  File "/usr/lib/python3.7/site-packages/nose/importer.py", line 47, in importFromPath
    return self.importFromDir(dir_path, fqname)
  File "/usr/lib/python3.7/site-packages/nose/importer.py", line 94, in importFromDir
    mod = load_module(part_fqname, fh, filename, desc)
  File "/usr/lib64/python3.7/imp.py", line 244, in load_module
    return load_package(name, filename)
  File "/usr/lib64/python3.7/imp.py", line 216, in load_package
    return _load(spec)
  File "<frozen importlib._bootstrap>", line 696, in _load
  File "<frozen importlib._bootstrap>", line 677, in _load_unlocked
  File "<frozen importlib._bootstrap_external>", line 728, in exec_module
  File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed
  File "/home/vagrant/devel/tests/__init__.py", line 37, in <module>
    import mock
ModuleNotFoundError: No module named 'mock'
----------------------------------------------------------------------
Ran 1 test in 0.001s
FAILED (errors=1)

Looks like you're also missing python3-mock no?

You are right, I installed the missing packages and found out, that black-3.7 has some changes from the black, that is provisioned in vagrant.

The tests should be passing now.

Let's put plugin earlier in the list of argument, because it is a required argument, it cannot be None in fact :)

Seeing how similar this method is to the one above, I wonder if we should try to merge their logic into a function that both API endpoints call. What do you think?

This doesn't say which plugin is active. If I enable multiple plugins, I have a list of {'active': True} but I don't know to which plugin they correspond :)

Proposal: List all the plugins installed on a project.

Proposal: List every plugins available in this pagure instance. For each plugin is provided their name as well as the name of the argument to provide to enable/disable them

Thinking more on this, do we even need two endpoints? Setting active to True should mean: install this plugin while setting it to False should mean remove/deactivate this plugin, so we could do both actions in a single endpoint, no?

Let's maybe shorten this response a bit, we don't need to cover all the existing plugins in the example. The endpoint does not need any authentication, not even a POST request, so anyone can go and check easily the actual output.

Let's put plugin earlier in the list of argument, because it is a required argument, it cannot be None in fact :)

Yes, you are right.

Seeing how similar this method is to the one above, I wonder if we should try to merge their logic into a function that both API endpoints call. What do you think?

Do you have some suggestion for name that is not confusing?

This doesn't say which plugin is active. If I enable multiple plugins, I have a list of {'active': True} but I don't know to which plugin they correspond :)

I should use the same output as in the api_view_plugins. I probably didn't realize that the name is actually missing.

Proposal: List all the plugins installed on a project.

This sounds better :-)

Thinking more on this, do we even need two endpoints? Setting active to True should mean: install this plugin while setting it to False should mean remove/deactivate this plugin, so we could do both actions in a single endpoint, no?

This was the logic how it is working in UI, my idea was to separate these two options, but I'm not sure now, if it is a good idea.

Let's maybe shorten this response a bit, we don't need to cover all the existing plugins in the example. The endpoint does not need any authentication, not even a POST request, so anyone can go and check easily the actual output.

This is just a test, I don't think it matters how long it is.

Let's maybe shorten this response a bit, we don't need to cover all the existing plugins in the example. The endpoint does not need any authentication, not even a POST request, so anyone can go and check easily the actual output.

This is just a test, I don't think it matters how long it is.

It shows up the API doc making it a bit long :)

You probably mean the sample output in api_view_call method, not the expected output in test.

Yes, I could definitely make this shorter.

You probably mean the sample output in api_view_call method, not the expected output in test.

Oups indeed, commented at the wrong place, sorry about that

Thinking more on this, do we even need two endpoints? Setting active to True should mean: install this plugin while setting it to False should mean remove/deactivate this plugin, so we could do both actions in a single endpoint, no?

This was the logic how it is working in UI, my idea was to separate these two options, but I'm not sure now, if it is a good idea.

There are two things which make me tick me currently, a) we really have a lot of code duplication, though we can easily fix that by moving the logic to its own function called by the two endpoints and b) we're sending the active=True/False argument regardless, so there is a bit of redundancy between active and the endpoint queried. Worst we could also have a mismatch there: if we were to send active=True to the /remove endpoint?

If we could drop the required active then I would be fine with two separate endpoints.

It's actually not required, I set it inside the function. If you send it, it will be replaced by correct value inside function.

It's actually not required, I set it inside the function. If you send it, it will be replaced by correct value inside function.

Then we should drop it from the documentation

You mean from the api_view_call? This is just returning every form field available and the active is one of them, but I think I could skip it.

You mean from the api_view_call? This is just returning every form field available and the active is one of them, but I think I could skip it.

yes but also from the /plugins as well as from the data returned when enabling/disabling it :)

Ok, I will remove it then.

I addressed all the issues in comments, rebased to latest master and moved same code to functions. One is new function, that will filter the fields, we don't want to send in response _filter_fields and the second similar code was moved to _check_plugin function in utils.py.

We'll need to blacklist plugins as name for a project, otherwise we will end up with URL conflicts (ie: if a project is created with the name plugins either you won't be able to access this endpoint or the project's descriptions).

Or we change this endpoint to something like _plugins since _ is not allowed as first characters for project's name.

One more note, the code (and especially the endpoint listing all the plugins) does not seem to honor the DISABLED_PLUGINS configuration key.

I added DISABLED_PLUGINS check to api_view_plugins function and changed the @API.route to /_plugins. This should address your comments.

I also rebased the branch to latest master.

FAILED test: py3-test_pagure_flask_api_plugins_view

The tests were not updated to the new URL? :)

@pingou I probably forgot to update them :-)

Tests are now fixed

s/12/3/ here I think :)

Should we check if the plugin is blacklisted here as well?

s/12/3/ here I think :)

You are right.

Should we check if the plugin is blacklisted here as well?

What blacklist do you have in mind?

I added new test for api_view_plugins for DISABLED_PLUGINS and fixed docstring for this function.

Should we check if the plugin is blacklisted here as well?

What blacklist do you have in mind?

I meant DISALBED_PLUGINS. We shouldn't be able to install a disabled hook.

This is already checked inside _check_plugin function. See https://pagure.io/pagure/pull-request/4669#_3__17

This is already checked inside _check_plugin function. See https://pagure.io/pagure/pull-request/4669#_3__17

Indeed thanks!

:thumbs_up:

Same for me, let's get this in

Thanks @zlopez !