Account Activation cleanup #22468

nasthagiri · 2019-12-08T21:54:19Z

This PR removes unused and unneeded endpoints and functionality related to account activation, specifically user_api's duplicative activate_account function, which was only used by tests. As part of this effort, a few surrounding tests and functionality were refactored and removed:

change_setting endpoint: Removed since it's not used anywhere in the codebase and not called in production.
user_authn.test_views.py: Refactored, moving password-related tests to test_password.py.

References:

nasthagiri · 2019-12-09T13:41:32Z

cms/djangoapps/contentstore/tests/tests.py

Note to Reviewers: The tests in this file were simplified to use the users created in ModuleStoreTestCase instead of needing to create and activate new ones. The focus of these tests are not to end-to-end test account creation/activation, but other functionality.

nasthagiri · 2019-12-09T13:43:01Z

common/djangoapps/student/tests/test_activate_account.py

Note to Reviewers: I updated these tests in order to merge test functionality that was in AccountActivationAndPasswordChangeTest in openedx/core/djangoapps/user_api/accounts/tests/test_api.py.

nasthagiri · 2019-12-09T13:44:27Z

common/djangoapps/student/urls.py

Note to Reviewers: I double-checked in New Relic that this API has been called 0 times since 6 weeks ago on Stage, Prod, and Edge. I believe it's a much older endpoint since user_api was created.

I'd like to move the remaining (email and activation) account-related views to user_api in a subsequent PR. Then, student will only contain functionality for the student dashboard and roles.

nasthagiri · 2019-12-09T13:47:59Z

common/djangoapps/student/views/management.py

Note to Reviewers: Thanks to @robrap's temporary metrics, we can verify that this CMS path is not needed. Also, @robrap's parallel effort to remove Studio's own account login/registration pages help eliminate this path.

I'm still seeing some /activate/ calls from cms (mostly Edge, but some Prod) which invokes this code.
See

SELECT request.uri, appName from Transaction where request.uri like '/activate/%' and appName like '%-cms'

And

SELECT * from Transaction where student_activate_account = 'cms'

I do not know why these are happening. Older activation emails? I do know the redirect to 'dashboard` definitely blows up when called from Studio.

Could we redirect /activate calls from cms to lms?

I removed the decoupled commit related to the removal of student_activate_account. That should unblock this PR.

nasthagiri · 2019-12-09T13:48:59Z

openedx/core/djangoapps/user_api/accounts/api.py

Note to Reviewers: Thanks to @robrap's metric here, we've verified that activate_account is used only in tests and not in production.

nasthagiri · 2019-12-09T13:51:06Z

openedx/core/djangoapps/user_authn/views/tests/test_logistration.py

Note to Reviewers: These tests are actually for testing Password Change functionality. I have moved them to test_password.py. Also, they do not need account-activation functionality for tests to pass.

dianakhuang

This looks good to me. Hooray for cleanup!

robrap

Great clean-up. I just have questions about activate_account_studio.

robrap · 2019-12-09T18:40:32Z

common/djangoapps/student/views/management.py

I'm still seeing some /activate/ calls from cms (mostly Edge, but some Prod) which invokes this code.
See

SELECT request.uri, appName from Transaction where request.uri like '/activate/%' and appName like '%-cms'

And

SELECT * from Transaction where student_activate_account = 'cms'

I do not know why these are happening. Older activation emails? I do know the redirect to 'dashboard` definitely blows up when called from Studio.

Could we redirect /activate calls from cms to lms?

robrap · 2019-12-09T19:06:12Z

openedx/core/djangoapps/user_authn/views/tests/test_logistration.py

Possible clean-up for a follow-up PR:
When looking at the file, I noticed the following assertNotContains: https://github.com/edx/edx-platform/blob/63a1a1db97e857d28f708117239440eb388c8758/openedx/core/djangoapps/user_authn/views/tests/test_logistration.py#L602
We had some tests that I fixed that used assertNotContains, but no parallel test with assertContains, and the thing that wasn't being found was because it was bogus code, not because some flag was working appropriately.

robrap

I'm marking as "Request Changes" to ensure my earlier comment about activate_account_studio is handled before merging. Thanks.

I also updated the subject. Unfortunately, once you have an Approval from someone, it looks pretty merge-able.

edx-status-bot · 2019-12-11T02:09:00Z

Your PR has finished running tests. There were no failures.

robrap

Nice!

edx-pipeline-bot · 2019-12-13T18:48:56Z

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production on Monday, December 16, 2019.

edx-pipeline-bot · 2019-12-16T15:15:50Z

EdX Release Notice: This PR has been deployed to the production environment.

nasthagiri requested review from a team December 8, 2019 21:54

nasthagiri force-pushed the arch/account-activation-cleanup branch 6 times, most recently from a3590b7 to 63a1a1d Compare December 9, 2019 04:12

nasthagiri commented Dec 9, 2019

View reviewed changes

nasthagiri assigned robrap and dianakhuang Dec 9, 2019

dianakhuang approved these changes Dec 9, 2019

View reviewed changes

robrap reviewed Dec 9, 2019

View reviewed changes

robrap requested changes Dec 9, 2019

View reviewed changes

robrap changed the title ~~Account Activation cleanup~~ [DO NOT MERGE] Account Activation cleanup Dec 9, 2019

nasthagiri added 3 commits December 10, 2019 20:23

student: Remove unused change_setting endpoint

90be249

user_api: Remove unneeded test-only activate_account

5e3df7a

user_authn: Move password-related tests to test_password.py

f539a51

nasthagiri force-pushed the arch/account-activation-cleanup branch from 63a1a1d to f539a51 Compare December 11, 2019 01:37

nasthagiri changed the title ~~[DO NOT MERGE] Account Activation cleanup~~ Account Activation cleanup Dec 11, 2019

nasthagiri merged commit 022a011 into master Dec 11, 2019

nasthagiri deleted the arch/account-activation-cleanup branch December 11, 2019 02:26

robrap reviewed Dec 11, 2019

View reviewed changes

Account Activation cleanup #22468

Account Activation cleanup #22468

Uh oh!

Conversation

nasthagiri commented Dec 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dianakhuang left a comment

Choose a reason for hiding this comment

Uh oh!

robrap left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robrap left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edx-status-bot commented Dec 11, 2019

Uh oh!

robrap left a comment

Choose a reason for hiding this comment

Uh oh!

edx-pipeline-bot commented Dec 13, 2019

Uh oh!

edx-pipeline-bot commented Dec 16, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

nasthagiri commented Dec 8, 2019 •

edited

Loading

robrap left a comment •

edited

Loading