Fix 500 when visiting /note with no noteid (#248) #1454
Conversation
weasyl/errorcode.py
Outdated
| @@ -98,6 +98,7 @@ | |||
| 'noGuests': no_guest_access, | |||
| "noCover": "No cover exists for that submission.", | |||
| "noImageSource": "No image exists from which to create a thumbnail.", | |||
| "noteidInvalid": "The specified note is invalid.", | |||
There was a problem hiding this comment.
I think a new error code for every invalid parameter is overkill. (It’s also not a 403.)
There was a problem hiding this comment.
Would you prefer a switch to the existing noteRecordMissing ("That note doesn't seem to be in our database."), and if so, would you want this to be a 404 or remain the default 422?
weasyl/forms.py
Outdated
| @@ -6,15 +6,16 @@ | |||
| _T = TypeVar("T") | |||
|
|
|||
|
|
|||
| def expect_id(s: str) -> int: | |||
| def expect_id(s: str, error_message="Unexpected") -> int: | |||
There was a problem hiding this comment.
No changes to this function, please. Keep it simple. isinstance(s, str) shouldn’t be checked on s: str.
weasyl/test/web/test_interaction.py
Outdated
| '/note?', | ||
| '/note?noteid', | ||
| '/note?noteid=', | ||
| '/note?noteid=a', |
There was a problem hiding this comment.
These tests are also overkill for something that’s more about integer parsing than notes.
There was a problem hiding this comment.
OK if I move these to a separate test_forms.py that just tests expect_id directly, or would you want most of these gone entirely?
There was a problem hiding this comment.
I’d still recommend just not fixing or testing this particular endpoint standalone – there’s nothing about /note that merits this special treatment as far as I’m aware. But if you could separate out test_forms and test_get_note_permissions into their own PRs, those would be great.
|
|
||
|
|
||
| @pytest.mark.usefixtures('db') | ||
| def test_get_note_permissions(app): |
There was a problem hiding this comment.
Thanks for adding this test! This is a good test.
| 'content': 'Content 2', | ||
| }, headers={'Cookie': cookie2}) | ||
|
|
||
| resp = app.get('/note?noteid=1', headers={'Cookie': cookie1}) |
There was a problem hiding this comment.
Can’t rely on these notes having a particular id. As far as how to find that id… exercising /notes might not be a bad thing?
|
|
||
| INVALID_PATHS = ( | ||
| '/note', | ||
| '/note?noteid=1¬eid=2', |
There was a problem hiding this comment.
Again, not really a thing that’s specific to notes. Regression tests are good in general, but extrapolating from this one, we will be testing basic parameter handling on every route.
There was a problem hiding this comment.
This is fair. I will remove the INVALID_PATHS-related tests and keep test_get_note_permissions.
| try: | ||
| noteid = forms.expect_id(request.GET.getone("noteid")) | ||
| except (KeyError, WeasylError) as e: |
There was a problem hiding this comment.
Nice try – this is about is good as it can be.
|
Closing this PR to split out the new tests into their own PRs. |
Split from the now-closed #1454. Reviewed-by: Charmander <~@charmander.me>
Fixes #248
I took the opportunity to practice writing tests for Weasyl by adding
weasyl.test.web.test_interaction.