Skip to content

fix: strip end slash if any when using a subpath - #36870

Merged
kdmccormick merged 2 commits into
openedx:masterfrom
holaontiveros:feat/OEXCOM-126-verified-account-redirection
Jun 9, 2025
Merged

fix: strip end slash if any when using a subpath#36870
kdmccormick merged 2 commits into
openedx:masterfrom
holaontiveros:feat/OEXCOM-126-verified-account-redirection

Conversation

@holaontiveros

@holaontiveros holaontiveros commented Jun 6, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Description

While trying to be enrolled on a verified course, the upgrade process get's to an error screen due to a double slash on the URL that it's added while doing the redirection to the account microfrontend

Screenshot of the error:
image

Video on how you get to the error:

Screen.Recording.2025-06-06.at.12.50.47.p.m.mov

This happens due to the base URL having an ending slash while also using a slash for the next part of the URL:

http://apps.local.openedx.io:1997/account//id-verification?course_id=course-v1%3Ademo%2BD01%2B2025_T1

(Notice the // before id-verification)

Given that the endslash is required for the base URL to work properly this PR intends to remove the endslash (if any) when also a sub-path it's being used next to it to prevent any issue even if there's slight URL changes later.

Afther this change the redirection works properly:

Screen.Recording.2025-06-06.at.12.48.35.p.m.mov

Supporting information

Fixes: openedx/wg-build-test-release#468

Testing instructions

Assumptions prior to testing:

  • Running dev environment
  • Branch for this PR is being used
  • You have a course that it's configured as require ID verification

Steps to test:

  • Access the list of available courses and locate your course tha requires id verif
  • Select the course / Click "Enroll Now" / Click "Upgrade now"

If everything is working correctly you should be properly redirect to the account verification screen.

Deadline

None

Other information

Include anything else that will help reviewers and consumers understand the change.

  • This changes doesn't depend on changes anywhere else
  • The only concern is that probably we need an overall change for this, like manage urls not as strings but as objects that will handle this kind of "edge cases"...

@openedx-webhooks

openedx-webhooks commented Jun 6, 2025

Copy link
Copy Markdown

Thanks for the pull request, @holaontiveros!

This repository is currently maintained by @openedx/wg-maintenance-edx-platform.

Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review.

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

  • If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
    • This process (including the steps you'll need to take) is documented here.
  • If it doesn't, simply proceed with the next step.
🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

  • Dependencies

    This PR must be merged before / after / at the same time as ...

  • Blockers

    This PR is waiting for OEP-1234 to be accepted.

  • Timeline information

    This PR must be merged by XX date because ...

  • Partner information

    This is for a course on edx.org.

  • Supporting documentation
  • Relevant Open edX discussion forum threads
🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

Details
Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

  • The size and impact of the changes that it introduces
  • The need for product review
  • Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Jun 6, 2025
@github-project-automation github-project-automation Bot moved this to Needs Triage in Contributions Jun 6, 2025
@holaontiveros
holaontiveros requested review from a team and Copilot June 6, 2025 20:40
Copilot AI reviewed Jun 6, 2025
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR ensures that any trailing slash on settings.ACCOUNT_MICROFRONTEND_URL is removed before appending subpaths, preventing double slashes in generated URLs.

  • Introduce rstrip('/') on ACCOUNT_MICROFRONTEND_URL in multiple modules
  • Update email template, verification views, services, and management command to use the trimmed base URL
  • Replace direct string concatenations with the trimmed variable

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
openedx/core/djangoapps/notifications/email/utils.py Trim base URL before building notification_settings_url
lms/djangoapps/verify_student/views.py Trim base URL before constructing reverify_url
lms/djangoapps/verify_student/services.py Trim base URL in get_verify_location
lms/djangoapps/verify_student/management/commands/send_verification_expiry_email.py Trim base URL in expiry email link
Comments suppressed due to low confidence (5)

openedx/core/djangoapps/notifications/email/utils.py:100

  • Variable name 'baseUrl' does not follow Python snake_case conventions; consider renaming it to 'base_url'.
baseUrl = settings.ACCOUNT_MICROFRONTEND_URL.rstrip('/')

lms/djangoapps/verify_student/views.py:1131

  • Variable name 'baseUrl' does not follow Python snake_case conventions; consider renaming it to 'base_url'.
baseUrl = settings.ACCOUNT_MICROFRONTEND_URL.rstrip('/')

lms/djangoapps/verify_student/services.py:254

  • Variable name 'baseUrl' does not follow Python snake_case conventions; consider renaming it to 'base_url'.
baseUrl = settings.ACCOUNT_MICROFRONTEND_URL.rstrip('/')

lms/djangoapps/verify_student/management/commands/send_verification_expiry_email.py:191

  • Variable name 'baseUrl' does not follow Python snake_case conventions; consider renaming it to 'base_url'.
baseUrl = settings.ACCOUNT_MICROFRONTEND_URL.rstrip('/')

lms/djangoapps/verify_student/services.py:255

  • New URL construction logic with trimmed base URL should have unit tests covering scenarios when ACCOUNT_MICROFRONTEND_URL is provided with and without a trailing slash.
location = f'{baseUrl}/id-verification'

Comment thread lms/djangoapps/verify_student/services.py Outdated
Returns URL for IDV on Account Microfrontend
"""
location = f'{settings.ACCOUNT_MICROFRONTEND_URL}/id-verification'
baseUrl = settings.ACCOUNT_MICROFRONTEND_URL.rstrip('/')

Copilot AI Jun 6, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The same trailing-slash stripping logic is duplicated across multiple modules; consider extracting this into a shared helper function to improve maintainability.

Suggested change
baseUrl = settings.ACCOUNT_MICROFRONTEND_URL.rstrip('/')
baseUrl = strip_trailing_slash(settings.ACCOUNT_MICROFRONTEND_URL)

Copilot uses AI. Check for mistakes.

@holaontiveros holaontiveros Jun 6, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean yeah but it's way better to address the problem in a more global way that probably doesn't even require using URLs as strings... 🤷‍♂️

@kdmccormick kdmccormick Jun 6, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

way better to address the problem in a more global way that probably doesn't even require using URLs as strings

absolutely.

As long as we're handing these as strings, though, copilot's advice seems pretty bad? We all know what .rstrip('/') means, why hide it behind a function?

kdmccormick
kdmccormick requested changes Jun 7, 2025
View reviewed changes

@kdmccormick kdmccormick left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a request to rename baseUrl. Otherwise, looks great, thanks for the fix and the great PR description.

Lastly-- please remove OEXCOM-126 from the commit message since that (Jira?) ticket isn't available to Open edX developers.

Comment thread lms/djangoapps/verify_student/management/commands/send_verification_expiry_email.py Outdated
@mariajgrimaldi mariajgrimaldi mentioned this pull request Jun 9, 2025
Closed
@holaontiveros holaontiveros changed the title fix(OEXCOM-126): strip end slash if any when using a subpath fix: strip end slash if any when using a subpath Jun 9, 2025
@holaontiveros

holaontiveros commented Jun 9, 2025

Copy link
Copy Markdown
Contributor Author

Just a request to rename baseUrl. Otherwise, looks great, thanks for the fix and the great PR description.

Lastly-- please remove OEXCOM-126 from the commit message since that (Jira?) ticket isn't available to Open edX developers.

Sure thing, I just had it like that given that it's how I had my local configured, I removed it from the title because I don't think it's on any commit, so I guess that's fine right?

Thanks for the review!

kdmccormick
kdmccormick approved these changes Jun 9, 2025
View reviewed changes

@kdmccormick kdmccormick left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, it's not on any commit. Thanks! Merging once tests pass.

@kdmccormick
kdmccormick enabled auto-merge (squash) June 9, 2025 14:46
@kdmccormick
kdmccormick merged commit 89e3d18 into openedx:master Jun 9, 2025
@github-project-automation github-project-automation Bot moved this from Needs Triage to Done in Contributions Jun 9, 2025
@holaontiveros
holaontiveros deleted the feat/OEXCOM-126-verified-account-redirection branch June 9, 2025 15:16
@mariajgrimaldi

mariajgrimaldi commented Jun 9, 2025

Copy link
Copy Markdown
Member

@holaontiveros: thank you so much for this! Can you help us with a backport to the release/teak branch? We'd really appreciate it! Thanks

@holaontiveros

holaontiveros commented Jun 9, 2025

Copy link
Copy Markdown
Contributor Author

@holaontiveros: thank you so much for this! Can you help us with a backport to the release/teak branch? We'd really appreciate it! Thanks

I'll work on it

holaontiveros added a commit to holaontiveros/edx-platform that referenced this pull request Jun 9, 2025
@holaontiveros
fix: strip end slash if any when using a subpath (openedx#36870)
db1131a 
While trying to be enrolled on a verified course, the upgrade process
gets to an error screen due to a double slash on the URL that it's added
while doing the redirection to the account microfrontend

Will be backported to Teak.
Part of openedx/wg-build-test-release#468
@holaontiveros holaontiveros mentioned this pull request Jun 9, 2025
Merged
@edx-pipeline-bot

edx-pipeline-bot commented Jun 9, 2025

Copy link
Copy Markdown
Contributor

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

@edx-pipeline-bot

edx-pipeline-bot commented Jun 9, 2025

Copy link
Copy Markdown
Contributor

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

@edx-pipeline-bot

edx-pipeline-bot commented Jun 9, 2025

Copy link
Copy Markdown
Contributor

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

@edx-pipeline-bot

edx-pipeline-bot commented Jun 9, 2025

Copy link
Copy Markdown
Contributor

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

1 similar comment
@edx-pipeline-bot

edx-pipeline-bot commented Jun 9, 2025

Copy link
Copy Markdown
Contributor

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

@edx-pipeline-bot

edx-pipeline-bot commented Jun 9, 2025

Copy link
Copy Markdown
Contributor

2U Release Notice: This PR has been rolled back from the edX production environment.

@edx-pipeline-bot

edx-pipeline-bot commented Jun 9, 2025

Copy link
Copy Markdown
Contributor

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

1 similar comment
@edx-pipeline-bot

edx-pipeline-bot commented Jun 9, 2025

Copy link
Copy Markdown
Contributor

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

@arslanashraf7 arslanashraf7 mentioned this pull request Jun 12, 2025
Merged
kdmccormick pushed a commit that referenced this pull request Jun 12, 2025
@arslanashraf7
fix: generate IDV URL only if ACCOUNT_MICROFRONTEND_URL is available (#…
d6dbc40 
…36898)

The api/courseware/course fails for all the verified enrollments if you
are not using Account MFE, which means that you probably won't set
ACCOUNT_MICROFRONTEND_URL in your settings/configurations.
So this PR adds a check safely try to do rstrip.

Fixes a bug in #36870
arslanashraf7 added a commit to mitodl/edx-platform that referenced this pull request Jun 17, 2025
@arslanashraf7
fix: generate IDV URL only if ACCOUNT_MICROFRONTEND_URL is available (o…
72cf7bf 
…penedx#36898)

The api/courseware/course fails for all the verified enrollments if you
are not using Account MFE, which means that you probably won't set
ACCOUNT_MICROFRONTEND_URL in your settings/configurations.
So this PR adds a check safely try to do rstrip.

Fixes a bug in openedx#36870
kdmccormick pushed a commit that referenced this pull request Jun 17, 2025
@arslanashraf7
fix: generate IDV URL only if ACCOUNT_MICROFRONTEND_URL is available (#…
b07add0 
…36924)

The api/courseware/course fails for all the verified enrollments if you
are not using Account MFE, which means that you probably won't set
ACCOUNT_MICROFRONTEND_URL in your settings/configurations.
So this PR adds a check safely try to do rstrip.

Fixes a bug in #36870
Backport of: d6dbc40
marlonkeating pushed a commit that referenced this pull request Jul 15, 2025
@holaontiveros @marlonkeating
fix: strip end slash if any when using a subpath (#36870)
08e1ebb 
While trying to be enrolled on a verified course, the upgrade process
gets to an error screen due to a double slash on the URL that it's added
while doing the redirection to the account microfrontend

Will be backported to Teak.
Part of openedx/wg-build-test-release#468
marlonkeating pushed a commit that referenced this pull request Jul 15, 2025
@arslanashraf7 @marlonkeating
fix: generate IDV URL only if ACCOUNT_MICROFRONTEND_URL is available (#…
5837c24 
…36898)

The api/courseware/course fails for all the verified enrollments if you
are not using Account MFE, which means that you probably won't set
ACCOUNT_MICROFRONTEND_URL in your settings/configurations.
So this PR adds a check safely try to do rstrip.

Fixes a bug in #36870
Copilot AI pushed a commit to AhtishamShahid/edx-platform that referenced this pull request Mar 4, 2026
@arslanashraf7 @AhtishamShahid
fix: generate IDV URL only if ACCOUNT_MICROFRONTEND_URL is available (o…
f08a93b 
…penedx#36924)

The api/courseware/course fails for all the verified enrollments if you
are not using Account MFE, which means that you probably won't set
ACCOUNT_MICROFRONTEND_URL in your settings/configurations.
So this PR adds a check safely try to do rstrip.

Fixes a bug in openedx#36870
Backport of: d6dbc40

(cherry picked from commit b07add0)
Copilot AI pushed a commit to AhtishamShahid/edx-platform that referenced this pull request Mar 4, 2026
@arslanashraf7
fix: generate IDV URL only if ACCOUNT_MICROFRONTEND_URL is available (o…
bd5dc33 
…penedx#36924)

The api/courseware/course fails for all the verified enrollments if you
are not using Account MFE, which means that you probably won't set
ACCOUNT_MICROFRONTEND_URL in your settings/configurations.
So this PR adds a check safely try to do rstrip.

Fixes a bug in openedx#36870
Backport of: d6dbc40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@kdmccormick kdmccormick kdmccormick approved these changes

Assignees

No one assigned

Labels

open-source-contribution PR author is not from Axim or 2U

Projects

Contributions
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Test failure] <TC_INSTRUCTOR_3>: <Problem with enrolling for a course with verified and professional enrollment tracks>

6 participants

@holaontiveros @openedx-webhooks @mariajgrimaldi @edx-pipeline-bot @kdmccormick