openwisp / openwisp-radius

Administration web interface and REST API for freeradius 3 build in django & python. Supports captive portal authentication, WPA Enerprise (802.1x), freeradius rlm_rest, social login, Hotspot 2.0 / 802.11u, importing users from CSV, registration of new users and more.
https://openwisp.io/docs/dev/radius/
GNU General Public License v3.0
364 stars 183 forks source link

[fix] Fixed 500 error when adding OrganizationUser and assigning RadiusGroup together #514

Closed Shiva953 closed 4 months ago

Shiva953 commented 8 months ago

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

Shiva953 commented 8 months ago

@pandafy could you provide me with some sample user data to add in the failing test case?

coveralls commented 8 months ago

Coverage Status

coverage: 98.669% (-0.05%) from 98.723% when pulling e13a3fef5573c638cd03d7093f677c2e1ff1bdc1 on Shiva953:issues/507-organizationuser-radiusgroup-error into 67fb11273444fe73018d7da40f139e787b26dee3 on openwisp:master.

Shiva953 commented 8 months ago

Looks better, please add a failing test case and don't forget to always follow the commit message style guidelines to allow the build to pass.

In which file under the tests directory should the test case be added for this function? would test_checks.py be an apt one?

Shiva953 commented 8 months ago

@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, test_default_group_handler can be a suitable name(to be added as method of TestUsersIntegration class)? Also, should we create another class for adding the test method?

pandafy commented 8 months ago

@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, test_default_group_handler can be a suitable name(to be added as method of TestUsersIntegration class)? Also, should we create another class for adding the test method?

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.

nemesifier commented 4 months ago

Superseded by #528.