[fix] Fixed 500 error when adding OrganizationUser and assigning RadiusGroup together #514
Conversation
Added a try-except block to ensure that it doesn't raise a 500 error and logs the apt error as warning whenever user tries adding OrganizationUser and assigning default RadiusGroup together. Fixes openwisp#507
|
@pandafy could you provide me with some sample user data to add in the failing test case? |
openwisp_radius/receivers.py
Outdated
| ug = RadiusUserGroup(user=instance.user, group=queryset.first()) | ||
| ug.full_clean() | ||
| ug.save() | ||
| except Exception as e: |
There was a problem hiding this comment.
We usually wrap in the try/except only the lines of code which we expect to fail and we must be specific in what exception we expect. This code can create more problems than what it's trying to solve because it can potentially hide other unwanted behavior. On operations like these which are not critical, letting them failing loudly is better than failing silently.
There was a problem hiding this comment.
PS: a failing test is missing
There was a problem hiding this comment.
We usually wrap in the try/except only the lines of code which we expect to fail and we must be specific in what exception we expect. This code can create more problems than what it's trying to solve because it can potentially hide other unwanted behavior. On operations like these which are not critical, letting them failing loudly is better than failing silently.
That makes sense, I think it would be better to wrap the try-except block inside the second nested if-statement instead, that being the part which prevents adding organization group and assigning radius group together. Is that a fair proposition?
There was a problem hiding this comment.
@Shiva953 that is correct.
Another thing, we don't want to catch all types of exceptions here. We shall check the exception class of the error raised (probably integrity error) and add handling only for that.
openwisp_radius/receivers.py
Outdated
| ug = RadiusUserGroup(user=instance.user, group=queryset.first()) | ||
| ug.full_clean() | ||
| ug.save() | ||
| except Exception as e: |
There was a problem hiding this comment.
@Shiva953 that is correct.
Another thing, we don't want to catch all types of exceptions here. We shall check the exception class of the error raised (probably integrity error) and add handling only for that.
Signed-off-by: Shiva953 <b22070@students.iitmandi.ac.in>
An empty commit to follow the commit conventions not followed in earlier commits, such that the QA checks pass. No new changes intended. Related to openwisp#507
Provides an apt error message description in case an exception arises. Fixes openwisp#507
In which file under the |
There was a problem hiding this comment.
@Shiva953, you need to add a test in https://github.com/openwisp/openwisp-radius/blob/master/openwisp_radius/tests/test_users_integration.py.
It was mentioned in the issue description.
sure, |
The test name sounds good to me. No, we don't need a new class for this test. You can add this to the existing class. I would also add a docstring to the text hinting why there was a need for such a test case. Basically, summarise the issue description in 1-2 lines. |
In this PR, I added a try-except block to ensure that whenever a new user is created, and edited by simultaneously adding organization user with "default" org & "default-users" RadiusGroup, an error is logged as warning instead of the server raising 500 error.
Fixes #507