1191808 - Change all the custom WebAPIError codes to be greater than 1000

Status: RESOLVED FIXED

(MozReview Graveyard :: General, defect, P2)

Description

[Mauro Doglio [:mdoglio]]

From https://bugzilla.mozilla.org/show_bug.cgi?id=1109218#c144:

General comment about all of the WebAPIError's introduced in the commits - All
error codes should be greater than 1000 (which RB core guarantees it will not
use itself). Also, I'm thinking it might just be best to stick them all in the
same file (including ones introduced by unrelated commits here). That way
finding a particular error to use, or incrementing to the next available id is
easier.

Comment 1

[Mauro Doglio [:mdoglio]]

Attachment: MozReview Request: Mozreview: WebAPIError codes greater than 1000 (Bug 1191808); r?smacleod

Mozreview: WebAPIError codes greater than 1000 (Bug 1191808); r?smacleod

Comment 2

[Mauro Doglio [:mdoglio]]

Attachment: MozReview Request: Mozreview: Add new WebAPIError AUTOLAND_CONFIGURATION_ERROR; r?smacleod

Mozreview: Add new WebAPIError AUTOLAND_CONFIGURATION_ERROR; r?smacleod

Comment 3

[Mauro Doglio [:mdoglio]]

Attachment: MozReview Request: Mozreview: substitute BAD_UPDATE_CREDENTIALS with PERMISSION_DENIED; r?smacleod

Mozreview: substitute BAD_UPDATE_CREDENTIALS with PERMISSION_DENIED; r?smacleod

Comment 4

[Steven MacLeod [:smacleod]]

Comment on attachment 8651802 [details]
MozReview Request: Mozreview: WebAPIError codes greater than 1000 (Bug 1191808); r?smacleod

https://reviewboard.mozilla.org/r/16987/#review15235

::: pylib/mozreview/mozreview/errors.py:13
(Diff revision 1)
>  

Can you update the commit message to include the reason for this change please.

Comment 5

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/16987/#review15237

::: pylib/mozreview/mozreview/autoland/resources.py:32
(Diff revision 1)
> -from mozreview.decorators import webapi_scm_groups_required
> +                              NOT_PUSHED_PARENT_REVIEW_REQUEST,

The trailing paren `)` should be added in this commit not the next, so this commit isn't broken.

Comment 6

[Steven MacLeod [:smacleod]]

Comment on attachment 8651803 [details]
MozReview Request: Mozreview: Add new WebAPIError AUTOLAND_CONFIGURATION_ERROR; r?smacleod

https://reviewboard.mozilla.org/r/16989/#review15239

::: pylib/mozreview/mozreview/autoland/resources.py:31
(Diff revision 1)
> -                              NOT_PUSHED_PARENT_REVIEW_REQUEST,
> +                              NOT_PUSHED_PARENT_REVIEW_REQUEST)

Make sure to move this change to the first commit.

::: pylib/mozreview/mozreview/errors.py:52
(Diff revision 1)
>  BAD_UPDATE_CREDENTIALS = WebAPIError(
>      1008,
>      "Bad or missing AutolandRequest update credentials.",
>      http_status=401)  # 401 Unauthorized

I don't believe 1008 is checked or used by anyone yet (and 1007 wasn't either). How about we just slip in a change to make this 1007 so there aren't any gaps :)

Comment 7

[Steven MacLeod [:smacleod]]

Comment on attachment 8651803 [details]
MozReview Request: Mozreview: Add new WebAPIError AUTOLAND_CONFIGURATION_ERROR; r?smacleod

https://reviewboard.mozilla.org/r/16989/#review15241

Meant to put a ship-it on the last review.

Comment 8

[Steven MacLeod [:smacleod]]

Comment on attachment 8651806 [details]
MozReview Request: Mozreview: substitute BAD_UPDATE_CREDENTIALS with PERMISSION_DENIED; r?smacleod

https://reviewboard.mozilla.org/r/16991/#review15243

::: pylib/mozreview/mozreview/autoland/resources.py:302
(Diff revision 1)
> -            return BAD_UPDATE_CREDENTIALS
> +            return PERMISSION_DENIED

Even though we're using the built in codes from review board for these things, you can still provide a custom message if you think it's useful.

See the method on `WebAPIError` and the example in its docstring here: https://github.com/djblets/djblets/blob/6c8dd31b0bd49e0d0ecad4cce1718b923fc4e7dd/djblets/webapi/errors.py#L52-L58

Comment 9

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/16989/#review15239

> I don't believe 1008 is checked or used by anyone yet (and 1007 wasn't either). How about we just slip in a change to make this 1007 so there aren't any gaps :)

Ah, I see in the next commit you remove this. NVM, carry on.

Comment 11

[Mauro Doglio [:mdoglio]]

Comment on attachment 8651802 [details]
MozReview Request: Mozreview: WebAPIError codes greater than 1000 (Bug 1191808); r?smacleod

Mozreview: WebAPIError codes greater than 1000 (Bug 1191808); r?smacleod

Error codes should be greater than 1000 (which RB core guarantees it will not
use itself). Let's  stick them all in the same file (including ones introduced
by unrelated commits here). That way finding a particular error to use, or
incrementing to the next available id is easier.

Comment 12

[Mauro Doglio [:mdoglio]]

Comment on attachment 8651803 [details]
MozReview Request: Mozreview: Add new WebAPIError AUTOLAND_CONFIGURATION_ERROR; r?smacleod

Mozreview: Add new WebAPIError AUTOLAND_CONFIGURATION_ERROR; r?smacleod

Comment 13

[Mauro Doglio [:mdoglio]]

Comment on attachment 8651806 [details]
MozReview Request: Mozreview: substitute BAD_UPDATE_CREDENTIALS with PERMISSION_DENIED; r?smacleod

Mozreview: substitute BAD_UPDATE_CREDENTIALS with PERMISSION_DENIED; r?smacleod