Properly handle bugzilla urls for unretirement requests #360
Conversation
| @@ -1972,15 +1975,21 @@ def add_new_package_request( | |||
| if pkg_collection.startswith(('el', 'epel')): | |||
| _validate_pkg(session, pkg_collection[-1:], pkg_name) | |||
|
|
|||
| if pkg_description: | |||
There was a problem hiding this comment.
This actually changes the behavior we had, if pkg_description is ' ' it will return True while we want None in fact
There was a problem hiding this comment.
I do not follow. Before my change pkgdb would use the empty string if pkg_description is just a space character:
In [3]: pkg_description = " "
In [4]: pkg_description.strip() if pkg_description else None
Out[4]: ''
There was a problem hiding this comment.
That's a bug then, it should be: pkg_description.strip() if pkg_description and pkg_description.strip() else None
There was a problem hiding this comment.
Is this only needed for the description or ofr the other fields for the review URL and upstream URL as well?
PkgdbBugzillaException is only defined in pkgdblib.exceptions not in pkgdblib. This makes the testsuite run without errors.
pkg_description, pkg_upstream_url and pkg_review_url should be None if they are empty, see: #360 (comment)
|
I believe I addressed all your suggestions now. |
| @@ -151,7 +151,7 @@ def api_admin_actions(): | |||
| status=status, | |||
| count=True, | |||
| ) | |||
| except pkgdblib.PkgdbException, err: # pragma: no cover | |||
| except pkgdblib.PkgdbException as err: # pragma: no cover | |||
There was a problem hiding this comment.
Here we use pkgdblib.PkgdbException but wasn't this the problem we were trying to address at first?
We adjusted api/acls.py to import the exception directly, should we adjust the others to behave in the same way?
There was a problem hiding this comment.
The problem was with PkgdbBugzillaException the PkgdbException is also availible in pkgdblib, because it is imported there. We can also make it the same in the other places, but it is going to extend the scope of this PR beyond its original goal.
There was a problem hiding this comment.
I realize it is available in pkgdblib, but somehow it was failing in the tests despite that.
So, imho, either we are consistent and fix it everywhere or figure out more precisely why the tests were failing
There was a problem hiding this comment.
So, imho, either we are consistent and fix it everywhere or figure out more precisely why the tests were failing
There are two different exceptions, one with Bugzilla it is name. This one caused the test failure. The changes to the exception withouth Bugzilla in its name was there to make it more consistent.
|
I'm wondering if we shouldn't split this PR into two so that we can merge the fix to bugzilla URLs on one side and deal with the exceptions in another PR |
|
I hope I got all exceptions and made made it uniform. If there are no other changes IMHO a second PR makes it only more complicated without much gain. |
You got them all in the API but missed the imports in the UI module, do you want me to do it?
I'm fine with keeping it all in one PR, I was suggesting splitting as a way to not stalling these changes :) |
Ok I must have had an old cache or so since I can now see the imports everywhere. Thanks for this PR, sorry it turned out to be that big :) |
|
👍 to merge |
|
Thank you |
This PR first fixes the test suite to not report errors because of undefined exceptions, adds a test for the function to identify the probem, fixes the problem and also fixes some PEP8 violations in the touched files.
This should fix the problem in https://fedorahosted.org/rel-eng/ticket/6426