pagure

#2484 New API endpoint to modify multiple custom fields.
Merged by pingou. Opened by cverna.
cverna/pagure multi_custom_key_api  into  master

Download 2484.patch

This commit adds a new endpoint so that a user can send multiple custom fields in one request.

In order to keep backward compatibility I have added a new route /custom and preserved the /custom/field_name route that can be used to edit a single field.

This PR has 3 commits, first one implement the new API, second refactor the common code between the 2 custom field API and the third commit remove a duplicated call to pagure.lib.add_metadata_update_notif

This partially fixes #2057

1 new commit added

  • Replace common logic with helper function to check link url

rebased

1 new commit added

  • Remove duplicated call to pagure.lib.add_metadata_update_notif

This sounds very close to EINVALIDREQ https://pagure.io/fork/cverna/pagure/blob/multi_custom_key_api/f/pagure/api/init.py#_64 no?

indentation?

This docblock isn't quite following the style we're using elsewhere :/

Since this is a dict, should we provide an example on how the dictionary should/could look like?

I think we're using messages as keys in the other endpoints

I'm not quire seeing the difference with the test just above :(

Oh, I see fields vs field

Let's maybe specify what is incorrect about this request

Is this needed? issue_tracker should be on by default no?

Should we add a test where the value provided for reviewstatus isn't in the list?

Quite a few comments but this is looking good :)

Yeah, I was looking for something similar, I ll use EINVALIDREQ.

OK I ll change it to match the others

I was thinking about it. Would you put the example in the description ?

Ok I will change it

Copy/paste :D

Ok, although I am not sure this is validated, I think you can add any value even if it not in the list, I think it is the same for the boolean ( you could add text). I can investigate and raise a new ticket if this is the case

1 new commit added

  • Fixing comment from the review

4 new commits added

  • Fixing comment from the review
  • Remove duplicated call to pagure.lib.add_metadata_update_notif
  • Replace common logic with helper function to check link url
  • New API endpoint to modify multiple custom fields.

rebased

I should have fixed all the comments. It is possible to add any value in the list field, there is no validation. I ll raise a ticket for this

4 new commits added

  • Fixing comment from the review
  • Remove duplicated call to pagure.lib.add_metadata_update_notif
  • Replace common logic with helper function to check link url
  • New API endpoint to modify multiple custom fields.

rebased

Not using the same style as the rest of the project here :(

Tests are failing (flake8)

So two comments, otherwise :thumbsup: for me

I followed the same style as the rest of the file :( https://pagure.io/pagure/blob/master/f/pagure/api/issue.py#_30

I don't mind changing the style but then it will be inconsistent in the file.

You're using ::param <key> : while the file users :param <key>: :)

Oops I was sure I fixed that :)

rebased

rebased

Ok I have update the docstring. I run the test on my side and no flake8 failure :(. Could double check and if it still fails tell me where it is failling ?

rebased

@pingou OK, I have rebased, run the tests and it looks good on my side.

rebased

Pull-Request has been merged by pingou
Metadata
None
No Tags
Static archive. pagure.io was sunset in 2026; this is a read-only snapshot.
Red Hat Logo

Fedora is sponsored by Red Hat. Learn more about the relationship between Red Hat and Fedora »

© Red Hat, Inc. and others.