Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Revert "feat: Account and profile MFE legacy removal - redeployment " #33542

Merged
merged 1 commit into from
Oct 19, 2023
Merged

Revert "feat: Account and profile MFE legacy removal - redeployment " #33542

merged 1 commit into from
Oct 19, 2023

Conversation

abdullahwaheed
Copy link
Contributor

Reverts #31893

@abdullahwaheed abdullahwaheed marked this pull request as ready for review October 19, 2023 07:20
@abdullahwaheed abdullahwaheed merged commit caf8e45 into master Oct 19, 2023
61 checks passed
@abdullahwaheed abdullahwaheed deleted the revert-31893-revert-31888-revert-30336-depr/account-pages branch October 19, 2023 07:26
@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

@kdmccormick
Copy link
Member

@abdullahwaheed @Syed-Ali-Abbas-Zaidi @Mashal-m Do you folks remember why this needed to be reverted?

@jristau1984
Copy link
Contributor

jristau1984 commented Feb 15, 2024 •
edited
Loading

@kdmccormick looking at NewRelic data for this deployment time, I see a spike in errors of the following:

builtins:ModuleNotFoundError
[prod-edx-edxapp-lms]
Error message: No module named 'openedx.core.djangoapps.user_api.accounts.toggles'

This error spike set off what looks like multiple alerts, which undoubtedly triggered the revert action. Sorry, but our NR data retention does not allow me to see more details about the error than what I pasted above.

I was able to dig this up from the corresponding Slack discussion:

Muhammad Abdullah Waheed Khan
[4 months ago]
actually it was failing to find module in django template, so it was causing a server error (500)

@kdmccormick
Copy link
Member

@abdullahwaheed Do you know what template it was?

@kdmccormick
Copy link
Member

I see a reference to in openedx.core.djangoapps.user_api.accounts.toggles in lms/templates/header/user_dropdown.html, but the PR removes that reference, so that shouldn't be the problem.

@Syed-Ali-Abbas-Zaidi
Copy link
Contributor

@kdmccormick, This same toggle is being used in edx-themes [ref], this was the reason why this PR got reverted. This toggle was not removed from edx-themes which caused the errors.

We also have this PR ready to remove this reference.

@Syed-Ali-Abbas-Zaidi
Copy link
Contributor

Also while digging into the issue, we found out that we need to handle all the legacy redirects of the profile and account page to redirect to the profile and account MFE instead. This PR can't go in until all the redirects are handled otherwise MFEs will break. FedBom started to create the PRs to handle the redirects but I don't know if FedBom managed to create all the PRs, @abdullahwaheed will have better knowledge about it.

Below are the PRs we created in profile and account MFE to handle the legacy redirects.

@feanil feanil mentioned this pull request Feb 27, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mashal-m Mashal-m Mashal-m approved these changes

@Syed-Ali-Abbas-Zaidi Syed-Ali-Abbas-Zaidi Syed-Ali-Abbas-Zaidi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@abdullahwaheed @edx-pipeline-bot @kdmccormick @jristau1984 @Syed-Ali-Abbas-Zaidi @Mashal-m