Pass empty list if there is no category id #166
Conversation
carkod
force-pushed
the
fix-blog-paginated-page-wd-6988
branch
2 times, most recently
from
6434997
to
6202b5d
Compare
carkod
force-pushed
the
fix-blog-paginated-page-wd-6988
branch
from
6202b5d
to
a0ddc2c
Compare
WordPress API returns 400 if return list with empty string, because it treats it as a Truly parameter. Instead we should pass an empty list, which will be accepted as Falsy in the case there is no id for the category.
carkod
force-pushed
the
fix-blog-paginated-page-wd-6988
branch
from
a0ddc2c
to
0b7b3d4
Compare
| }, | ||
| ) | ||
| except HTTPError as error: | ||
| if error.response.status_code == 400: |
There was a problem hiding this comment.
what about other codes? will they just silently break?
There was a problem hiding this comment.
Other 40X errors not likely to occur (e.g. if 403 happens then it should fail and not be handled because there must have been some change on server side that we need to update like tokens or secrets or firewall, or 402 similar error codes forbidden access are unusual and indicate something changed) this is a 400 bad request which is returned by the API endpoint and we need to handle as a result.
500 errors should break and they will show up in Sentry for us to fix and people should report it.
There was a problem hiding this comment.
I would have wanted to even skip the status_code but then it fails because there is no json. So I wanted to be as specific as possible to avoid catching unwanted errors
There was a problem hiding this comment.
okay, if you are sure that in all other error cases than 400 this will still work as expected, meaning it will report to Sentry and there won't be any weird behavior on the client side, then good to go
There was a problem hiding this comment.
Yes, in this specific case, we don't want it to break, because it's just somebody randomly entering a number that exceeds the limit. In theory WordPress API should return a 404, but for some reason they API specification doesn't determine that is how it should be.
| }, | ||
| ) | ||
| except HTTPError as error: | ||
| if error.response.status_code == 400: |
There was a problem hiding this comment.
okay, if you are sure that in all other error cases than 400 this will still work as expected, meaning it will report to Sentry and there won't be any weird behavior on the client side, then good to go
|
@carkod Ant merged something similar, might be worth rebasing and bumping to 6.4.2 if you want to further improve on this. |
mm latest version seems to be |
|
Ok looks like it's fixed in the latest version |
WordPress API returns 400 if return list with empty string, because it treats it as a Truly parameter. Instead we should pass an empty list, which will be accepted as Falsy in the case there is no id for the category.
QA
Go to https://ubuntu-com-13283.demos.haus/blog?page=2, check that it doesn't break anymore https://ubuntu.com/blog?page=2
Go to https://ubuntu-com-13283.demos.haus/blog?page=500, check that it return 404 now (exceeded max num of pages)
or
requirements.txtof ubuntu.com project to:dotrun clean && dotrunIssues
Fixes #158
Fixes https://warthogs.atlassian.net/browse/WD-6988