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

feat: add course authoring mfe base url to env vars #182

Closed
wants to merge 1 commit into from
Closed

feat: add course authoring mfe base url to env vars #182

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/20240108_150445_romulo_add_course_authoring_mfe_base_url.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
- [Improvement] Adds the "COURSE_AUTHORING_MFE_BASE_URL" env var pointing to the Course Authoring MFE address. (by @rpenido)
1 change: 1 addition & 0 deletions tutormfe/patches/openedx-lms-development-settings
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ MFE_CONFIG["ACCOUNT_SETTINGS_URL"] = ACCOUNT_MICROFRONTEND_URL
{% if get_mfe("course-authoring") %}
MFE_CONFIG["ENABLE_NEW_EDITOR_PAGES"] = True
MFE_CONFIG["ENABLE_PROGRESS_GRAPH_SETTINGS"] = True
MFE_CONFIG["COURSE_AUTHORING_MFE_BASE_URL"] = "http://{{ MFE_HOST }}:{{ get_mfe("course-authoring")["port"] }}"
Copy link

Choose a reason for hiding this comment

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

@rpenido Shouldn't this be "http://{{ MFE_HOST }}:{{ get_mfe("course-authoring")["port"] }}/course-authoring" ? That seems more consistent with all of the other MFE _URL variables in this file.

Also, there seems to be some inconsistency about whether these have a trailing slash or not, but most of them (except account and learner-dashboard) do not so that's probably the way to go.

Copy link
Contributor Author

@rpenido rpenido Jan 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

To give some context, we are using this in the iFrame.
We are using the COURSE_AUTHORING_MFE_BASE_URL here: https://github.com/openedx/frontend-app-library-authoring/pull/400/files

We have the url /course-authoring in the COURSE_AUTHORING_MICROFRONTEND_URL variable already set here:

tutor-mfe/tutormfe/patches/openedx-cms-development-settings

Line 2 in 19efd6c

COURSE_AUTHORING_MICROFRONTEND_URL = "http://{{ MFE_HOST }}:{{ get_mfe('course-authoring')["port"] }}/course-authoring"

But this URL renders an error to me:
http://apps.local.edly.io:2001/course-authoring/home
image

The course authoring URL in my tutor instance is the following:
image

Is there some misconfiguration on my side?

PS: Feel free to close this is this is not the right way to fix it

Copy link

Choose a reason for hiding this comment

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

It's actually a bug in the course authoring MFE: openedx/frontend-app-authoring#784

"Studio Home" should be showing at http://apps.local.overhang.io:2001/course-authoring/home (or edly.io)

So I think COURSE_AUTHORING_MICROFRONTEND_URL is all you need, and you can close this PR.

{% endif %}

{% if get_mfe("discussions") %}
Expand Down
1 change: 1 addition & 0 deletions tutormfe/patches/openedx-lms-production-settings
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ MFE_CONFIG["ACCOUNT_SETTINGS_URL"] = ACCOUNT_MICROFRONTEND_URL
{% if get_mfe("course-authoring") %}
MFE_CONFIG["ENABLE_NEW_EDITOR_PAGES"] = True
MFE_CONFIG["ENABLE_PROGRESS_GRAPH_SETTINGS"] = True
MFE_CONFIG["COURSE_AUTHORING_MFE_BASE_URL"] = "http://{{ MFE_HOST }}:{{ get_mfe("course-authoring")["port"] }}"
{% endif %}

{% if get_mfe("discussions") %}
Expand Down