feat: replace hardcoded edx string with site_name from configs #425

ihor-romaniuk · 2023-01-05T14:38:30Z

Description:

This PR replace the hardcoded edx string in the page head with the SITE_NAME value from the configuration.

Related PRs:

PR to the olive brunch - #424

openedx-webhooks · 2023-01-05T14:38:34Z

Thanks for the pull request, @ihor-romaniuk! 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.

codecov · 2023-01-12T15:35:12Z

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.04 🎉

Comparison is base (1344c28) 74.48% compared to head (3dbcf5f) 74.53%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #425      +/-   ##
==========================================
+ Coverage   74.48%   74.53%   +0.04%     
==========================================
  Files         106      108       +2     
  Lines        2085     2089       +4     
  Branches      514      514              
==========================================
+ Hits         1553     1557       +4     
  Misses        504      504              
  Partials       28       28

Impacted Files	Coverage Δ
src/index.jsx	`0.00% <ø> (ø)`
src/head/Head.jsx	`100.00% <100.00%> (ø)`
src/head/messages.js	`100.00% <100.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

kdmccormick · 2023-01-12T18:09:59Z

public/index.html

@@ -1,7 +1,7 @@
 <!doctype html>
 <html lang="en-us">
  <head>
-      <title>Course Authoring | edX</title>
+      <title>Course Authoring | <%= process.env.SITE_NAME %></title>


@arbrandes This uses static configuration (process.env.SITE_NAME) instead of runtime configuration (getConfig().SITE_NAME). Am I right in presuming that runtime config isn't available in this file?

Yes, you are right, just a static configuration. Also, this approach is used by all MFEs.

@ihor-romaniuk, right. However, Kyle's point is relevant: in order to truly fix the problem, we'll have to introduce a way to get the SITE_NAME dynamically.

We can use frontend-app-account as a template. See the <Head /> component that was introduced there for this very purpose. It makes use of the <Helmet /> component to modify the page title with the dynamically-obtained site name.

Do you mind doing that here as well? Otherwise, the change is not very useful to current users.

mphilbrick211 · 2023-01-23T21:26:37Z

Hi @KristinAoki! Flagging this for your review.

arbrandes · 2023-01-24T10:47:46Z

public/index.html

@@ -1,7 +1,7 @@
 <!doctype html>
 <html lang="en-us">
  <head>
-      <title>Course Authoring | edX</title>
+      <title>Course Authoring | <%= process.env.SITE_NAME %></title>


@ihor-romaniuk, right. However, Kyle's point is relevant: in order to truly fix the problem, we'll have to introduce a way to get the SITE_NAME dynamically.

We can use frontend-app-account as a template. See the <Head /> component that was introduced there for this very purpose. It makes use of the <Helmet /> component to modify the page title with the dynamically-obtained site name.

Do you mind doing that here as well? Otherwise, the change is not very useful to current users.

mphilbrick211 · 2023-02-07T00:10:51Z

Hi @ihor-romaniuk! Just checking to see if the requested changes were made and/or that this pull request is ready for review. Thanks!

mphilbrick211 · 2023-02-22T19:35:28Z

Hi @ihor-romaniuk! Friendly check-in on this :)

ihor-romaniuk · 2023-03-02T18:01:20Z

Hi @arbrandes, @kdmccormick
We are definitely right. I added ability to obtain site name dynamically as you suggested.
Also, It required localization of overriding ReactDOM.createPortal for allowing portals in jest.

Everything was updated and it's ready for review.

PR #424 was also updated.

mphilbrick211 · 2023-03-03T03:38:01Z

@arbrandes @kdmccormick @jristau1984 would you mind taking a look at this? Thanks!

jristau1984 · 2023-03-03T13:31:00Z

Tests failed. It may have been a flaky run, so I have kicked them off again.

jristau1984 · 2023-03-03T16:31:21Z

@arbrandes @ihor-romaniuk CI is still failing due to changes requested.

ihor-romaniuk · 2023-03-06T08:14:27Z

Hi @jristau1984. All checkers are green.

mphilbrick211 · 2023-03-07T01:21:17Z

@ihor-romaniuk @arbrandes - are the changes on this all set? Can @jristau1984 merge?

arbrandes

Awesome, thanks for the changes, @ihor-romaniuk!

openedx-webhooks · 2023-03-09T18:38:28Z

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

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Jan 5, 2023

ihor-romaniuk mentioned this pull request Jan 5, 2023

feat: replace hardcoded edx string in page head #424

Merged

mphilbrick211 added the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Jan 6, 2023

e0d removed the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Jan 12, 2023

kdmccormick reviewed Jan 12, 2023

View reviewed changes

arbrandes mentioned this pull request Jan 18, 2023

Reviews for sprint 2023-01-09 - 2023-01-22 openedx/axim-engineering#595

Closed

22 tasks

arbrandes self-requested a review January 20, 2023 17:26

arbrandes mentioned this pull request Jan 23, 2023

Adolfo's reviews (2023-01-23) openedx/axim-engineering#620

Closed

44 tasks

arbrandes self-assigned this Jan 23, 2023

mphilbrick211 requested a review from KristinAoki January 23, 2023 21:26

arbrandes requested changes Jan 24, 2023

View reviewed changes

mphilbrick211 added changes requested waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Jan 27, 2023

arbrandes mentioned this pull request Feb 6, 2023

Adolfo's reviews (2023-02-06) openedx/axim-engineering#655

Closed

22 tasks

arbrandes mentioned this pull request Mar 1, 2023

Adolfo's reviews (2023-02-20) openedx/axim-engineering#688

Closed

ihor-romaniuk added 2 commits March 2, 2023 18:21

feat: replace hardcoded edx string with site_name from configs

6d72656

feat: add ability to obtain site name dynamically

820f556

ihor-romaniuk force-pushed the romaniuk/feat/replace-hardcoded-edx-string branch from 119bbe3 to 820f556 Compare March 2, 2023 16:42

fix: localize overriding createPortal method

3dbcf5f

mphilbrick211 requested review from arbrandes, kdmccormick and jristau1984 March 3, 2023 03:37

mphilbrick211 removed the changes requested label Mar 3, 2023

mphilbrick211 removed the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Mar 3, 2023

kdmccormick removed their request for review March 6, 2023 14:21

arbrandes mentioned this pull request Mar 8, 2023

Adolfo's reviews openedx/axim-engineering#696

Closed

arbrandes approved these changes Mar 9, 2023

View reviewed changes

jristau1984 merged commit 5c0ca7b into openedx:master Mar 9, 2023

ihor-romaniuk deleted the romaniuk/feat/replace-hardcoded-edx-string branch March 13, 2023 15:28

bradenmacdonald pushed a commit to open-craft/frontend-app-authoring that referenced this pull request Aug 17, 2024

fix: add validation to problem number fields (openedx#425)

940482d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: replace hardcoded edx string with site_name from configs #425

feat: replace hardcoded edx string with site_name from configs #425

ihor-romaniuk commented Jan 5, 2023

openedx-webhooks commented Jan 5, 2023

codecov bot commented Jan 12, 2023 •

edited

Loading

kdmccormick Jan 12, 2023

ihor-romaniuk Jan 22, 2023 •

edited

Loading

arbrandes Jan 24, 2023

mphilbrick211 commented Jan 23, 2023

arbrandes Jan 24, 2023

mphilbrick211 commented Feb 7, 2023

mphilbrick211 commented Feb 22, 2023

ihor-romaniuk commented Mar 2, 2023 •

edited

Loading

mphilbrick211 commented Mar 3, 2023

jristau1984 commented Mar 3, 2023

jristau1984 commented Mar 3, 2023

ihor-romaniuk commented Mar 6, 2023

mphilbrick211 commented Mar 7, 2023

arbrandes left a comment

openedx-webhooks commented Mar 9, 2023

feat: replace hardcoded edx string with site_name from configs #425

feat: replace hardcoded edx string with site_name from configs #425

Conversation

ihor-romaniuk commented Jan 5, 2023

Description:

Related PRs:

openedx-webhooks commented Jan 5, 2023

codecov bot commented Jan 12, 2023 • edited Loading

Codecov Report

kdmccormick Jan 12, 2023

Choose a reason for hiding this comment

ihor-romaniuk Jan 22, 2023 • edited Loading

Choose a reason for hiding this comment

arbrandes Jan 24, 2023

Choose a reason for hiding this comment

mphilbrick211 commented Jan 23, 2023

arbrandes Jan 24, 2023

Choose a reason for hiding this comment

mphilbrick211 commented Feb 7, 2023

mphilbrick211 commented Feb 22, 2023

ihor-romaniuk commented Mar 2, 2023 • edited Loading

mphilbrick211 commented Mar 3, 2023

jristau1984 commented Mar 3, 2023

jristau1984 commented Mar 3, 2023

ihor-romaniuk commented Mar 6, 2023

mphilbrick211 commented Mar 7, 2023

arbrandes left a comment

Choose a reason for hiding this comment

openedx-webhooks commented Mar 9, 2023

codecov bot commented Jan 12, 2023 •

edited

Loading

ihor-romaniuk Jan 22, 2023 •

edited

Loading

ihor-romaniuk commented Mar 2, 2023 •

edited

Loading