[fix] Fixed - Exception raised on opening non existing user ID #228 #233
Conversation
There was a problem hiding this comment.
Good work @Saurav-Shrivastav ! 👍
I have requested some minor changes, see my comments below.
Saurav-Shrivastav
force-pushed
the
issues/228-fix-exception-raised
branch
from
0dbbe47
to
b3c91db
Compare
|
Thanks for the suggestions @pandafy. I've implemented them. |
There was a problem hiding this comment.
@Saurav-Shrivastav I manually tested the code today found a discrepancy. While you have achieved what was described in the issue description, that breaks the normal flow of Django Admin.
The change_view method of django.contrib.admin.ModelAdmin is implemented in a way that if an object does not exists or if validation fails, the user is redirected to the admin index page.
See following screenshots which I took while testing similar behaviour with change_view of organization model.
With the current patch, the user will be greeted with a 404 Not Found page if the corresponding object of User model does not exists or fails Validation.
I have shared suggestions to improve the current patch so it follows typical flow of Django Admin.
Saurav-Shrivastav
force-pushed
the
issues/228-fix-exception-raised
branch
2 times, most recently
from
9b83c43
to
a215ab5
Compare
Saurav-Shrivastav
force-pushed
the
issues/228-fix-exception-raised
branch
from
a215ab5
to
e18277d
Compare
Saurav-Shrivastav
force-pushed
the
issues/228-fix-exception-raised
branch
from
e18277d
to
ba566eb
Compare
openwisp_users/tests/test_admin.py
Outdated
| # check for version < 3.1 | ||
| content = f'User with ID "{id}" doesn\'t exist. Perhaps it was deleted?' | ||
| else: | ||
| content = f'User with ID “{id}” doesn’t exist. Perhaps it was deleted?' |
There was a problem hiding this comment.
I would have avoided the version switch and checked only for part of the string, however, since it's already done and the check works, we can leave it, thanks.
There was a problem hiding this comment.
Do you mean something like this?
content = '<li class="warning">User with ID 'I did think about this, but then this might lead to us missing out on checking the keywords like WRONG or the invalid-id.
Saurav-Shrivastav
force-pushed
the
issues/228-fix-exception-raised
branch
from
ba566eb
to
e64feec
Compare
openwisp_users/tests/test_admin.py
Outdated
| content = f'User with ID "{id}" doesn\'t exist. Perhaps it was deleted?' | ||
| else: | ||
| content = f'User with ID “{id}” doesn’t exist. Perhaps it was deleted?' | ||
| response.content = html.unescape(response.content.decode()) |
There was a problem hiding this comment.
You don't need to do this here. You can remove it.
There was a problem hiding this comment.
Are you referring to line no 202?
There was a problem hiding this comment.
It is required for Django 2.2, since it converts HTML entities like " to "
There was a problem hiding this comment.
Thanks @Saurav-Shrivastav, but support for django 2.2 was already planned to be dropped, the PR was ready but not merged yet, now is merged: #199.
There was a problem hiding this comment.
Thanks @Saurav-Shrivastav, but support for django 2.2 was already planned to be dropped, the PR was ready but not merged yet, now is merged: #199.
…sp#228 - Added a test to check the response status on navigating to a non-existing user change page - Handled exceptions for an invalid UUID and a non-existing user Fixes openwisp#228
Saurav-Shrivastav
force-pushed
the
issues/228-fix-exception-raised
branch
from
e64feec
to
abfccb3
Compare
|
@nemesisdesign I have removed the portion of code that checked the django version, looks good now ^^ |
|
Thanks @Saurav-Shrivastav 👍 |
Fixes #228