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

fix: convert variables that replace mfe .env into string #202

Closed
wants to merge 1 commit into from
Closed

fix: convert variables that replace mfe .env into string #202

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
8 changes: 4 additions & 4 deletions tutormfe/patches/openedx-lms-development-settings
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ MFE_CONFIG = {
{% if get_mfe("authn") %}
AUTHN_MICROFRONTEND_URL = "http://{{ MFE_HOST }}:{{ get_mfe("authn")["port"] }}/authn"
AUTHN_MICROFRONTEND_DOMAIN = "{{ MFE_HOST }}/authn"
MFE_CONFIG["DISABLE_ENTERPRISE_LOGIN"] = True
MFE_CONFIG["DISABLE_ENTERPRISE_LOGIN"] = "true"
{% endif %}

{% if get_mfe("account") %}
Expand All @@ -36,8 +36,8 @@ MFE_CONFIG["ACCOUNT_SETTINGS_URL"] = ACCOUNT_MICROFRONTEND_URL
{% endif %}

{% if get_mfe("course-authoring") %}
MFE_CONFIG["ENABLE_NEW_EDITOR_PAGES"] = True
MFE_CONFIG["ENABLE_PROGRESS_GRAPH_SETTINGS"] = True
MFE_CONFIG["ENABLE_NEW_EDITOR_PAGES"] = "true"
MFE_CONFIG["ENABLE_PROGRESS_GRAPH_SETTINGS"] = "true"
MFE_CONFIG["COURSE_AUTHORING_MICROFRONTEND_URL"] = "http://{{ MFE_HOST }}:{{ get_mfe("course-authoring")["port"] }}/course-authoring"
{% endif %}

Expand Down Expand Up @@ -71,7 +71,7 @@ MFE_CONFIG["ACCOUNT_PROFILE_URL"] = "http://{{ MFE_HOST }}:{{ get_mfe("profile")

{% if get_mfe("communications") %}
COMMUNICATIONS_MICROFRONTEND_URL = "http://{{ MFE_HOST }}:{{ get_mfe("communications")["port"] }}/communications"
MFE_CONFIG["SCHEDULE_EMAIL_SECTION"] = True
MFE_CONFIG["SCHEDULE_EMAIL_SECTION"] = "true"
{% endif %}

# Cors configuration
Expand Down
8 changes: 4 additions & 4 deletions tutormfe/patches/openedx-lms-production-settings
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ MFE_CONFIG = {
{% if get_mfe("authn") %}
AUTHN_MICROFRONTEND_URL = "{% if ENABLE_HTTPS %}https://{% else %}http://{% endif %}{{ MFE_HOST }}/authn"
AUTHN_MICROFRONTEND_DOMAIN = "{{ MFE_HOST }}/authn"
MFE_CONFIG["DISABLE_ENTERPRISE_LOGIN"] = True
MFE_CONFIG["DISABLE_ENTERPRISE_LOGIN"] = "true"
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here. Can you please share why are we converting to string here as well? I checked this here. There is no comparison done against a string.

{% endif %}

{% if get_mfe("account") %}
Expand All @@ -37,8 +37,8 @@ MFE_CONFIG["ACCOUNT_SETTINGS_URL"] = ACCOUNT_MICROFRONTEND_URL
{% endif %}

{% if get_mfe("course-authoring") %}
MFE_CONFIG["ENABLE_NEW_EDITOR_PAGES"] = True
MFE_CONFIG["ENABLE_PROGRESS_GRAPH_SETTINGS"] = True
MFE_CONFIG["ENABLE_NEW_EDITOR_PAGES"] = "true"
MFE_CONFIG["ENABLE_PROGRESS_GRAPH_SETTINGS"] = "true"
MFE_CONFIG["COURSE_AUTHORING_MICROFRONTEND_URL"] = "{% if ENABLE_HTTPS %}https://{% else %}http://{% endif %}{{ MFE_HOST }}/course-authoring"
{% endif %}

Expand Down Expand Up @@ -72,7 +72,7 @@ MFE_CONFIG["ACCOUNT_PROFILE_URL"] = "{% if ENABLE_HTTPS %}https://{% else %}http

{% if get_mfe("communications") %}
COMMUNICATIONS_MICROFRONTEND_URL = "{% if ENABLE_HTTPS %}https://{% else %}http://{% endif %}{{ MFE_HOST }}/communications"
MFE_CONFIG["SCHEDULE_EMAIL_SECTION"] = True
MFE_CONFIG["SCHEDULE_EMAIL_SECTION"] = "true"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we replacing bool to string here? We are not comparing anywhere it with "true" here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks a lot for the review. I changed all the bools with strings in this PR because they would all trip with the same issue with process.env returning strings and MFE_CONFIG returning bool.
I might have overreached as I did not test all of them.

Given the latest round of comments in the course-authoring PR I think we might close this PR altogether. I'll update this PR after we settle the discussion there.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have gone through the course-authoring discussion and just got that the issue was in Quince 3. Is that resolved now so I can close this PR?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The reason for replacing all bool to strings is that this would be the theoretically correct behavior for all MFE_CONFIG variables. I'll reproduce what I wrote on another PR:

getConfig() still returns values as they were set by the .env files, unless they were overriden by runtime config. Since .env files are still supported, and the dotenv package that reads them in doesn't support booleans, we have no choice but to assume everything that getConfig() spits out is a string.

The problem is that some MFEs don't follow this assumption correctly. For instance, to use your example search, take a look here:

https://github.com/openedx/frontend-app-communications/blob/9279a3e4ce888f6cc198ecb63c0638bf8a795667/src/components/bulk-email-tool/bulk-email-form/BulkEmailForm.jsx#L307

This line is actually wrong. The correct check would be getConfig().SCHEDULE_EMAIL_SECTION === "true". What it's actually doing now is just checking to see if the variable is empty of not. Setting it to "False" in a .env file would not have the expected results.

All of this said, we'll just have to live with some inconsistencies in tutor-mfe until everything conforms to the same standard upstream.

{% endif %}

LOGIN_REDIRECT_WHITELIST.append("{{ MFE_HOST }}")
Expand Down
Loading