fix: removing ENABLE_NEW_EDITOR_PAGES flag

[felipemontoya]

Description

The comparison in the routes files is === 'true' while getConfig().ENABLE_NEW_EDITOR_PAGES contains a Boolean.

Supporting information

Fixes #835.

Other information

This is a high priority as it was listed as one of the primary blocker of the release Redwood

[openedx-webhooks]

Thanks for the pull request, @felipemontoya! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

• supporting documentation
• Open edX discussion forum threads
• timeline information ("this must be merged by XX date", and why that is)
• partner information ("this is a course on edx.org")
• any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

[arbrandes]

Was this it? Great find!

However, I'm concerned that we may have a consistency problem. If you grep for === 'true' elsewhere in this repository, you'll find a bunch of instances.

[arbrandes]

Not only on this repository, but others. I believe this comes from the .env files. After quick research, it seems all env variables are supposed to be strings - no booleans - so it seems that checking for 'true' is correct.

Source:

• Boolean and null values motdotla/dotenv#51
• https://stackoverflow.com/questions/10265208/node-js-process-env-assigning-process-env-property-to-undefined-results-in-stri/10265271#10265271

I believe that at least for now we should be fixing this on the other end: tutor-mfe.

(When we remove support for the .env files we can revisit this, as both mfe_config and JS config would support actual booleans.)

[felipemontoya]

Yes, consistency is indeed a problem.
I think if when we were comparing process.env.ENABLE_TEAM_TYPE_SETTING === 'true' the process.env was always returning a string in each key. No longer so with getConfig().

Here is the result of logging getConfig() to the console. Plenty of both "true" as str and true as bool.

ACCESS_TOKEN_COOKIE_NAME: "edx-jwt-cookie-header-payload"
ACCOUNT_PROFILE_URL: "http://apps.local.edly.io:1995/profile"
ACCOUNT_SETTINGS_URL: "http://apps.local.edly.io:1997/account/"
APP_ID: "course-authoring"
AUTHN_MINIMAL_HEADER: false
BASE_URL: "apps.local.edly.io"
BBB_LEARN_MORE_URL: ""
CALCULATOR_HELP_URL: null
COURSE_AUTHORING_MICROFRONTEND_URL: "http://apps.local.edly.io:2001/course-authoring"
CREDENTIALS_BASE_URL: ""
CSRF_TOKEN_API_PATH: "/csrf/api/v1/token"
DISABLE_ENTERPRISE_LOGIN: true
DISCOVERY_API_BASE_URL: ""
DISCUSSIONS_MFE_BASE_URL: "http://apps.local.edly.io:2002/discussions"
ECOMMERCE_BASE_URL: "http://localhost:18130"
ENABLE_ACCESSIBILITY_PAGE: "false"
ENABLE_ASSETS_PAGE: "false"
ENABLE_CHECKLIST_QUALITY: "true"
ENABLE_HOME_PAGE_COURSE_API_V2: false
ENABLE_NEW_EDITOR_PAGES: true
ENABLE_OPEN_MANAGED_TEAM_TYPE: true
ENABLE_PROGRESS_GRAPH_SETTINGS: true
ENABLE_TAGGING_TAXONOMY_PAGES: "true"
ENABLE_TEAM_TYPE_SETTING: false
ENABLE_UNIT_PAGE: "false"
ENABLE_VIDEO_UPLOAD_PAGE_LINK_IN_CONTENT_DROPDOWN: "false"
ENVIRONMENT: "development"
EXAMS_BASE_URL: null
FAVICON_URL: "http://local.edly.io/favicon.ico"
IGNORED_ERROR_REGEX: undefined
INFO_EMAIL: "contact@local.edly.io"
LANGUAGE_PREFERENCE_COOKIE_NAME: "openedx-language-preference"
LEARNING_BASE_URL: "http://apps.local.edly.io:2000"
LMS_BASE_URL: "http://local.edly.io:8000"
LOGIN_URL: "http://local.edly.io:8000/login"
LOGOUT_URL: "http://local.edly.io:8000/logout"
LOGO_TRADEMARK_URL: "http://local.edly.io:8000/theming/asset/images/logo.png"
LOGO_URL: "http://local.edly.io:8000/theming/asset/images/logo.png"
LOGO_WHITE_URL: "http://local.edly.io:8000/theming/asset/images/logo.png"
MARKETING_SITE_BASE_URL: "http://local.edly.io:8000"
MFE_CONFIG_API_URL: "/api/mfe_config/v1"
NOTIFICATION_FEEDBACK_URL: null
ORDER_HISTORY_URL: "localhost:1996/orders"
PASSWORD_RESET_SUPPORT_LINK: "mailto:contact@local.edly.io"
PRIVACY_POLICY_URL: null
PUBLIC_PATH: "/course-authoring/"
PUBLISHER_BASE_URL: ""
REFRESH_ACCESS_TOKEN_ENDPOINT: "http://local.edly.io:8000/login_refresh"
SCHEDULE_EMAIL_SECTION: true
SECURE_COOKIES: false
SEGMENT_KEY: "null"
SITE_NAME: "master branch"
STUDIO_BASE_URL: "http://studio.local.edly.io:8001"
STUDIO_SHORT_NAME: "Studio"
SUPPORT_EMAIL: null
SUPPORT_URL: "https://support.edx.org"
TERMS_OF_SERVICE_URL: null
USER_INFO_COOKIE_NAME: "user-info"

[felipemontoya]

for now we should be fixing this on the other end: tutor-mfe.

I though this was being processed by the new_core_editors.use_new_text_editor waffle flag. That is why I made the change here. However turning that waffle flag off did not change the behavior of the MFE. It did change the behavior of the button in studio that takes one to either the MFE or the old edit experience:

Given that the feature is still controllable via the waffle flag, would it not be better to remove the "ENABLE_NEW_EDITOR_PAGES" check altogether?
If the user reaches the route because studio sent them there, the MFE should render properly. From a real user perspective nobody is going to write the URL by hand and since this is a proper feature I also don't think it is an attack vector of any sort.

[arbrandes]

No longer so with getConfig().

That's not quite right. 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.

would it not be better to remove the "ENABLE_NEW_EDITOR_PAGES" check altogether?

I think it's worth looking at, yes! We just need to be sure there's no real use case for ENABLE_NEW_EDITOR_PAGES=false.

@GlugovGrGlib, do you happen to have any thoughts on this one?

[bradenmacdonald]

I'm not sure why our booleans are so inconsistent, but until the issue is definitely resolved at the config level, I recommend working around it with this sort of approach:

frontend-app-course-authoring/src/header/Header.jsx

Line 25 in 2641aec

const meiliSearchEnabled = [true, 'true'].includes(getConfig().MEILISEARCH_ENABLED);

[felipemontoya]

getConfig() still returns values as they were set by the .env files, unless they were overriden by runtime

This definitely makes the case for tutor-mfe to set the values just as they where in the .env. This is happening in a bunch of variables, not only ENABLE_NEW_EDITOR_PAGES.

I would still remove the check in the case of ENABLE_NEW_EDITOR_PAGES, but I also created a PR at tutor-mfe overhangio/tutor-mfe#202 to solve all the instances I found

[GlugovGrGlib]

I think it's worth looking at, yes! We just need to be sure there's no real use case for ENABLE_NEW_EDITOR_PAGES=false.

@felipemontoya @arbrandes I fully agree that we should remove ENABLE_NEW_EDITOR_PAGES as well as ENABLE_UNIT_PAGE to simplify configuration process for the unit page. It allows us to manage redirect to the unit page solely through the waffle flag in edx-platform.

[felipemontoya]

Thanks Glib. @arbrandes is that decided then? should I update this PR removing the checks?

I think we should still continue with the tutor-mfe PR and make sure it lands, but depending on the outcome of this PR I will update that one to have only the necessary values.

[arbrandes]

@felipemontoya, I think as far as this PR goes, we should simply remove the checks, yes. This will fix the problem on this side.

As for the more general discussion, I think we should take it to the tutor-mfe PR.

[arbrandes]

Perfect!

[arbrandes]

Wait, actually, can you also remove all instances of ENABLE_NEW_EDITOR_PAGES in this repo? Including .env files, the README, and index.jsx? Thank you!

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.02%. Comparing base (99a144a) to head (1e32166).
Report is 27 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #951      +/-   ##
==========================================
+ Coverage   92.00%   92.02%   +0.02%     
==========================================
  Files         612      686      +74     
  Lines       10746    11953    +1207     
  Branches     2305     2596     +291     
==========================================
+ Hits         9887    11000    +1113     
- Misses        830      917      +87     
- Partials       29       36       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[felipemontoya]

@arbrandes good catch. It is nice to make a PR that mostly removes unnecessary code

[arbrandes]

Yes, exactly! And thanks!

It seems you missed one, though: https://github.com/openedx/frontend-app-course-authoring/blob/master/src/index.jsx#L121

[felipemontoya]

Ninja ammend. Now they are all gone.

[openedx-webhooks]

@felipemontoya 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.