implementation of delete profile for users without usable passwords (shibboleth, allauth social accounts, ..) #601
Conversation
|
Hi @MyPyDavid |
MyPyDavid
force-pushed
the
feature/599-add-shibboleth-delete-profile
branch
from
1c48da5
to
6b34930
Compare
MyPyDavid
force-pushed
the
feature/599-add-shibboleth-delete-profile
branch
from
86bef01
to
c72d0db
Compare
There was a problem hiding this comment.
Hi @MyPyDavid, thanks a lot for this contribution. I reviewed your changes and added some minor questions what might be changed. For the LDAP-case this implementation works analogue to the Shibboleth-case, i.e. without prompting for password and only by entering the e-mail address. I am not sure if it is a requirement that we should ask for the password in LDAP-case as it is currently.
rdmo/accounts/tests/test_utils.py
Outdated
| username = 'user' | ||
| email = 'user@example.com' | ||
| db_user = get_user_model().objects.get(username=username, email=email) | ||
| assert get_user_from_db_or_none(username, email) == db_user |
There was a problem hiding this comment.
Probably, we might add tests that if either the username or the e-mail does not correspond to the same user. Because I think the current implementation in get_user_from_db_or_none() allows to have multiple users with the same e-mail if the username is different.
There was a problem hiding this comment.
yes, I've added a testcase with a loop here:
rdmo/rdmo/accounts/tests/test_utils.py
Line 77 in 02874ae
There was a problem hiding this comment.
In this case it is much nicer to use @pytest.mark.parametrize to create seperate tests for the different users automatically, e.g.
rdmo/rdmo/accounts/tests/test_viewsets.py
Line 36 in 02874ae
There was a problem hiding this comment.
I've added a bunch of @pytest.mark.parametrize to the tests.
| @@ -258,6 +291,36 @@ def test_remove_user_post(db, client): | |||
| assert not get_user_model().objects.filter(username='user').exists() | |||
|
|
|||
|
|
|||
| def test_remove_user_post_cancelled_when_update_is_true(db, client, settings): | |||
| settings.PROFILE_UPDATE = True | |||
| if settings.PROFILE_DELETE: | |||
There was a problem hiding this comment.
Same here, should we add tests so that all combinations are tested: Update True/False and Delete True/False? Maybe this is a general question for our tests. I think it would make sense to try to test all combinations in order not to forget something due to the local/default test config.
There was a problem hiding this comment.
yes think so too. In general, I would like to have the option to loop through these different test configs/combinations as well.. I can look into that but don't know if it will be too much for this PR.
There was a problem hiding this comment.
for now, I will keep it so that the settings are set to True/False at the start of each test case. We can keep in mind to refactor/improve the tests with that in another PR but also that the amount of test cases should be kept maintainable.
There was a problem hiding this comment.
I've added now a True/False option with @pytest.mark.parametrize('test_setting', boolean_toggle) to most of the test cases. Then in each test case, only the relevant part is checked in a conditional eg:
if settings.PROFILE_UPDATE:
assertTemplateUsed(response, 'profile/profile_update_form.html')
else:
assertTemplateUsed(response, 'profile/profile_update_closed.html')
|
Hi @MyPyDavid and @m6121 , thanks a lot. Looks ok to me. |
MyPyDavid
force-pushed
the
feature/599-add-shibboleth-delete-profile
branch
2 times, most recently
from
9950d75
to
c4946bb
Compare
MyPyDavid
changed the title
There was a problem hiding this comment.
Awesome! Thanks a lot @MyPyDavid for your work on the tests and the implementation. Looks good to me.
|
alright! thanks for the review and discussions @m6121 and @jochenklar ! |
| assert response.url == reverse('account_reset_password_from_key', args=['4','set-password']) | ||
| # get the link from the mail | ||
| # urls = re.findall(r'http[s]?://(?:[a-zA-Z]|[0-9]|[$-_@.&+]|[!*\(\),]|(?:%[0-9a-fA-F][0-9a-fA-F]))+', mail.outbox[0].body) # complicated regex | ||
| urls = [i.strip() for i in mail.outbox[0].body.splitlines() if i.strip().startswith('http')] # simpler alternative |
There was a problem hiding this comment.
should we leave this complicated regex:
urls = re.findall(r'http[s]?://(?:[a-zA-Z]|[0-9]|[$-_@.&+]|[!*\(\),]|(?:%[0-9a-fA-F][0-9a-fA-F]))+', mail.outbox[0].body) # complicated regexor this in here?
urls = [i.strip() for i in mail.outbox[0].body.splitlines() if i.strip().startswith('http')] # simpler alternative…-shibboleth-delete-profile implementation of delete profile for users without usable passwords (shibboleth, allauth social accounts, ..)
Changes for #599:
Added functionalities for when
user.has_usable_password()isFalse, this holds for users from shibboleth or from allauth social accounts. Did not(need?)to useif settings.SHIBBOLETHfor this implementation.RemoveFormdelete_userfuncremove_userViewuser.has_usable_password()is True for them @m6121