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.

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!: footer legal links #403

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

asadali145
Copy link
Contributor

@asadali145 asadali145 commented Jan 25, 2024

Related Issues

Product Review Issue: openedx/platform-roadmap#324
mitodl/mitxonline#223

Description

Legacy LMS provides a way to configure the footer links but the MFE footer has no support for legal links. This PR adds support to configure custom legal links in the MFE footer.

Testing

  • This PR can be tested with the Accounts MFE https://github.com/openedx/frontend-app-account.
  • Test without adding the links. The footer should be the same as before.
  • Add config links in .env.development:
    • ACCESSIBILITY_URL
    • ABOUT_US_URL
    • HONOR_CODE_URL
    • CONTACT_URL
    • SUPPORT_CENTER_URL
    • SUPPORT_CENTER_TEXT
    • TRADEMARK_TEXT
    • FOOTER_LOGO_ALT_TEXT
    • SHOW_FOOTER_LOGO
    • TERMS_OF_SERVICE_URL
    • PRIVACY_POLICY_URL
  • Verify the footer links in the MFE
  • Verify the Accessibility URL in the Studio MFE

Screenshots

  1. MFE Footer after our changes
Screenshot 2024-02-22 at 4 32 31 PM
  1. MFE Footer before these changes
300859871-205c0ce3-8c0a-4c74-b6e9-dde2233af4b3
  1. Legacy LMS Footer:
300857051-ee5005ec-4a6c-40c6-ba10-18ca61005b79
  1. Studio MFE footer
300857498-cf882470-bd2a-4997-9202-ecb7cd885ebb

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Jan 25, 2024
@openedx-webhooks
Copy link

openedx-webhooks commented Jan 25, 2024

Thanks for the pull request, @asadali145!

What's next?

Please work through the following steps to get your changes 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.

🔘 Let us know that your PR is ready for review:

Who will review my changes?

This repository is currently unmaintained.

To get help with finding a technical reviewer, tag the community contributions project manager for this PR in a comment and let them know that your changes are ready for review:

  1. On the right-hand side of the PR, find the Contributions project, click the caret in the top right corner to expand it, and check the "Primary PM" field for the name of your PM.
  2. Find their GitHub handle here.

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.

@mphilbrick211
Copy link

Hi @asadali145! When you get a moment, would you mind putting a brief description of this contribution at the top of the PR? Thank you!

@asadali145
Copy link
Contributor Author

@mphilbrick211 Sure, I am just testing my changes with a couple of MFEs. Will add the description once I move it to the review.

@asadali145 asadali145 changed the title feat: footer legal links feat!: footer legal links Feb 6, 2024
@asadali145 asadali145 changed the title feat!: footer legal links feat: footer legal links Feb 6, 2024
@asadali145 asadali145 marked this pull request as ready for review February 6, 2024 11:47
@asadali145
Copy link
Contributor Author

@mphilbrick211 This is ready now.

Copy link

@pdpinch pdpinch left a comment

Choose a reason for hiding this comment

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

I'm not too familiar with how this works, but I have a couple of questions.

Also, are these settings -- both existing and newly added -- documented anywhere?

.env.development Outdated
@@ -12,7 +12,6 @@ TERMS_OF_SERVICE_URL=null
PRIVACY_POLICY_URL=null
SUPPORT_EMAIL=null
STUDIO_BASE_URL=http://localhost:18010
SHOW_ACCESSIBILITY_PAGE=false
Copy link

Choose a reason for hiding this comment

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

Why did you remove this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have tried to make it consistent with other links. The accessibility link was redirecting to <studio_base_url>/accessibility if SHOW_ACCESSIBILITY_PAGE=true. I have changed it and added a config ACCESSIBILITY_URL. Now, it will display the link if ACCESSIBILITY_URL is set and will redirect to the same URL.

Choose a reason for hiding this comment

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

Should we also look for other usages of this variable in all other MFEs that are using it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

MFEs will be updated once they update the header and footer versions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Correct. But this means that this change is a breaking one, so after squashing the commits, the one that remains should be a "feat!:".

.env.development Outdated
Comment on lines 24 to 32
ACCESSIBILITY_URL=
ABOUT_US_URL=
HONOR_CODE_URL=
CONTACT_URL=
SUPPORT_CENTER_URL=
SUPPORT_CENTER_TEXT=
TRADEMARK_TEXT=
LOGO_ALT_TEXT=
SHOW_LOGO=
Copy link

Choose a reason for hiding this comment

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

Is there any order to these settings?

@asadali145
Copy link
Contributor Author

Also, are these settings -- both existing and newly added -- documented anywhere?

@pdpinch A couple of required configs are mentioned in the readme but not all. I can add the newly added under a new heading if you want.

Copy link

@arslanashraf7 arslanashraf7 left a comment

Choose a reason for hiding this comment

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

This is just a code review, I haven't tested it out yet.

README.rst Outdated
Comment on lines 48 to 49
Apart from the required environment variables, this component also supports the following optional environment variable. These variables add the ability to display
custom legal links in the footer. Optional Environment Variables can also be set by the consuming micro-frontend

Choose a reason for hiding this comment

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

Suggested change
Apart from the required environment variables, this component also supports the following optional environment variable. These variables add the ability to display
custom legal links in the footer. Optional Environment Variables can also be set by the consuming micro-frontend
Apart from the required environment variables, this component also supports the following optional environment variables. These variables add the ability to display
custom legal links in the footer. Optional environment variables can also be set by the micro-frontend.

.env.development Outdated
SITE_NAME=Open edX
SUPPORT_CENTER_TEXT=

Choose a reason for hiding this comment

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

Shouldn't these be set to empty strings ''? This applied to other variables as well e.g. TRADEMARK_TEXT

.env.development Outdated
CREDENTIALS_BASE_URL=http://localhost:18150
CSRF_TOKEN_API_PATH=/csrf/api/v1/token
ECOMMERCE_BASE_URL=http://localhost:18130
HONOR_CODE_URL=

Choose a reason for hiding this comment

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

Should we use a standard name for this? e.g. TOS_AND_HONOR_CODE. This is the one that we are already using in authn MFE .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is already a URL for the TOS page, this is meant to be a separate URL.

.env.development Outdated
@@ -12,7 +12,6 @@ TERMS_OF_SERVICE_URL=null
PRIVACY_POLICY_URL=null
SUPPORT_EMAIL=null
STUDIO_BASE_URL=http://localhost:18010
SHOW_ACCESSIBILITY_PAGE=false

Choose a reason for hiding this comment

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

Should we also look for other usages of this variable in all other MFEs that are using it?

.env.development Outdated
USER_INFO_COOKIE_NAME=edx-user-info
LOGO_ALT_TEXT="Open edX Logo"

Choose a reason for hiding this comment

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

Do we need to make it configurable? There exists another existing variable but that has the name HEADER_LOGO_ALT_TEXT. e..g https://github.com/search?q=org%3Aopenedx+LOGO_ALT_TEXT&type=code. Should we name it FOOTER_LOGO_ALT_TEXT?

return (
<footer
role="contentinfo"
className="footer d-flex border-top py-3 px-4"
>
<div className="container-fluid d-flex">
{ process.env.SHOW_LOGO

Choose a reason for hiding this comment

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

I think we should use getConfig from frontend-platform. An example of that is can be seen in dashboard MFE

Comment on lines 78 to 84
{this.renderLinkIfExists(process.env.ABOUT_US_URL, 'About Us')}
{this.renderLinkIfExists(process.env.TERMS_OF_SERVICE_URL, 'Terms of Service')}
{this.renderLinkIfExists(process.env.PRIVACY_POLICY_URL, 'Privacy Policy')}
{this.renderLinkIfExists(process.env.HONOR_CODE_URL, 'Honor Code')}
{this.renderLinkIfExists(process.env.CONTACT_URL, 'Contact Us')}
{this.renderLinkIfExists(process.env.ACCESSIBILITY_URL, 'Accessibility')}
{this.renderLinkIfExists(process.env.SUPPORT_CENTER_URL, process.env.SUPPORT_CENTER_TEXT || 'FAQ & Help')}

Choose a reason for hiding this comment

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

We should use formatMessage with internationalization. An example of that can be seen in dashboard MFE

@@ -14,7 +14,7 @@ exports[`<Footer /> renders correctly renders with a language selector 1`] = `
href="http://localhost:18000"
>
<img
alt="Powered by Open edX"

Choose a reason for hiding this comment

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

Not sure why we removed this.

.env.development Outdated
ACCESS_TOKEN_COOKIE_NAME=edx-jwt-cookie-header-payload
ACCESSIBILITY_URL=

Choose a reason for hiding this comment

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

I think the accessibility is a mandatory page for a website to make it compliant with legalities. I think we should have a default fallback value for this. e.g. how we are making sure in StudioFooter. By default, we are sending the user to STUDIO_URL/accessibility.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am unsure why they added studio_url/accessibility as a default because no such path exists with the default installations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you want then I can add it to the required configs.

src/components/studio-footer/StudioFooter.jsx Show resolved Hide resolved
@sarina
Copy link
Contributor

sarina commented Feb 20, 2024

Could you please upload images in your PR description that aren't broken? I only see a broken image link. Thanks!

@ali-hugo ali-hugo self-requested a review February 20, 2024 15:21
@pdpinch
Copy link

pdpinch commented Feb 21, 2024

@asadali145 I opened a Product Review issue to go with this, openedx/platform-roadmap#324 -- can you add it to the PR description?

@ali-hugo
Copy link

Could you please upload images in your PR description that aren't broken? I only see a broken image link. Thanks!

@asadali145 +1 to Sarina's request. Also, it would be helpful if you could upload a screencast of the user flow inside Studio (I'd like to perform a product review on this PR, but I'm not a developer so I don't know how to set up a dev environment to do the review). Thanks!

@asadali145
Copy link
Contributor Author

@asadali145 I opened a Product Review issue to go with this, openedx/platform-roadmap#324 -- can you add it to the PR description?

@pdpinch I have updated the PR description with the Product Review Issue link.

Could you please upload images in your PR description that aren't broken? I only see a broken image link. Thanks!

@ali-hugo & @sarina I have updated the screenshots. Please check.

Also, it would be helpful if you could upload a screencast of the user flow inside Studio (I'd like to perform a product review on this PR, but I'm not a developer so I don't know how to set up a dev environment to do the review). Thanks!

@ali-hugo What do you mean by the user flow inside the studio? Would you like me to add Authoring MFE screenshots?

@asadali145
Copy link
Contributor Author

This is ready for review.

CC: @pdpinch @arslanashraf7

@ali-hugo
Copy link

@asadali145

I have updated the screenshots. Please check.

Thank you.

What do you mean by the user flow inside the studio? Would you like me to add Authoring MFE screenshots?

I think that's what I need (sorry, I don't know where the user adds and manages the footer links). Basically, what I would like to see is the process the operator has to follow to add/edit/remove the links (unless this happens inside the code, in which case I won't need to review it). I hope that makes sense!

@asadali145
Copy link
Contributor Author

I think that's what I need (sorry, I don't know where the user adds and manages the footer links). Basically, what I would like to see is the process the operator has to follow to add/edit/remove the links (unless this happens inside the code, in which case I won't need to review it). I hope that makes sense!

Yeah, now I got your point. In this case, the links are configured through code. DevOps teams can configure these links by adding the required configurations through the code.

@ali-hugo
Copy link

ali-hugo commented Feb 26, 2024

@asadali145 Thanks for confirming that the links are added in the code.

I am meeting with other members of the product team tomorrow. I would like to get their feedback on this feature before I do my review. I'll get back to you once I have consolidated their thoughts.


EDIT:
Product review process started here.

@ali-hugo
Copy link

ali-hugo commented Feb 27, 2024

@itsjeyd @mphilbrick211 I have an OSPR process question for you; I was asked to perform a product review on this PR but just noticed that it is not tagged as "ready for product review". Would it be best for me to hold off until that tag has been added? I don't want to interfere with the process you've put in place.


EDIT:
We discussed this in the Core Product Working Group meeting, and decided that I should wait for the "ready for product review" tag to be applied (sorry for any confusion @asadali145. You may have noticed that a product discussion has been started here).

@sarina
Copy link
Contributor

sarina commented Feb 27, 2024

@ali-hugo fyi! https://docs.openedx.org/en/latest/developers/references/developer_guide/process/pull-request-statuses.html

@jmakowski1123 jmakowski1123 added the product review PR requires product review before merging label Feb 27, 2024
@jmakowski1123
Copy link

Thanks to @pdpinch for pulling this PR into the product review process. I've started the review process here: openedx/platform-roadmap#324 (comment) - would love for other stakeholders to chime in.

@asadali145 asadali145 force-pushed the asad/footer-legal-links branch 2 times, most recently from 05da7e7 to 963b141 Compare March 1, 2024 09:24
@asadali145
Copy link
Contributor Author

@arbrandes Could you please have another look?

<div className="copyright-col">
{getConfig().TRADEMARK_TEXT
&& (
<div className="trademark-text text-gray-500 small">
Copy link
Contributor

Choose a reason for hiding this comment

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

I still think text-gray-500 is too specific to be hard-coded. Maybe remove it and reimplement it in the CSS, so it can be overriden more explicitly by theming later?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was a bootstrap mixin. I have updated it now.

@asadali145 asadali145 force-pushed the asad/footer-legal-links branch 2 times, most recently from b82b5e7 to 1b6de13 Compare March 1, 2024 13:16
@arbrandes
Copy link
Contributor

Ok, I'm going to actually try it out shortly, and if it works as expected I'll approve!

@ali-hugo
Copy link

ali-hugo commented Mar 4, 2024

@itsjeyd Thank you for the detailed explanation.

Although that page only talks about what to do for product proposals that the community submits before writing any code. It doesn't specify what to do for PRs that need product review but haven't had a proposal submitted first.

Yes, that's where my confusion comes in. I will broach this topic with the Core Product Working Group and see if we can get a procedure put in place for reviewing PR's that don't have product proposals. At some point I may pull you and Michelle into the conversation for your recommendations.

Thanks for your help on this.

@arbrandes
Copy link
Contributor

arbrandes commented Mar 4, 2024

@ali-hugo, please let me know what the proper procedure ends up being: I expect to be finding a bunch of PRs in the "needs-product-review-but-doesn't-have-a-proposal" category in the near future.

@pdpinch
Copy link

pdpinch commented Mar 5, 2024

@itsjeyd @ali-hugo I opened openedx/platform-roadmap#324 -- does that qualify as a product proposal?

@sarina
Copy link
Contributor

sarina commented Mar 5, 2024

"needs-product-review-but-doesn't-have-a-proposal" category

I'll double-check but I think it's very simple: if it needs product review, everyone should stop (code reviewing/developing) the PR, make the proposal, and go through the product review process. It's a waste of (code/pr) reviewer's time to be looking at something that doesn't have product review.

@itsjeyd
Copy link

itsjeyd commented Mar 5, 2024

@ali-hugo

I will broach this topic with the Core Product Working Group and see if we can get a procedure put in place for reviewing PR's that don't have product proposals. At some point I may pull you and Michelle into the conversation for your recommendations.

Sounds great, thanks @ali-hugo!

@itsjeyd
Copy link

itsjeyd commented Mar 5, 2024

@pdpinch

I opened openedx/platform-roadmap#324 -- does that qualify as a product proposal?

The feature ticket LGTM, thanks for creating it. I'm not part of the product working group, though, and don't have all the necessary context to answer your question definitively.

CC @ali-hugo @mphilbrick211

@ali-hugo
Copy link

ali-hugo commented Mar 6, 2024

Hi @pdpinch,

We are still in the process of defining the product proposal / review process, so apologies for anything that's unclear.

I opened openedx/platform-roadmap#324 -- does that qualify as a product proposal?

I'm going to have to ask @jmakowski1123 to confirm this ^^.

In the meantime, here's what I do know: there are two different templates for product proposals. Please visit this link, select the template that best fits your use case, and compare the requirements listed in the template with the information included in your proposal. Your proposal seems to include many of the requirements already, but you might be able to flesh it out a bit in places (e.g. by including any UX/UI designs).

Sorry I couldn't provide a definite answer, but I'm sure @jmakowski1123 will be able to.

@ali-hugo
Copy link

ali-hugo commented Mar 6, 2024

@arbrandes

@ali-hugo, please let me know what the proper procedure ends up being: I expect to be finding a bunch of PRs in the "needs-product-review-but-doesn't-have-a-proposal" category in the near future.

We're in the process of defining the procedure in this document. Please keep an eye on that to see how the decisions unfold. Feel free to add any comments there too.

@ali-hugo ali-hugo removed their request for review March 15, 2024 12:34
Add the ability to configure footer links.

BREAKING CHANGE: SHOW_ACCESSIBILITY_PAGE is replaced with the ACCESSIBILITY_URL config.
---------

Co-authored-by: Wassaf-Shahzad <[email protected]>
Co-authored-by: Arslan Ashraf <[email protected]>
@asadali145
Copy link
Contributor Author

Ok, I'm going to actually try it out shortly, and if it works as expected I'll approve!

@arbrandes Do you have an update on testing? I have rebased my PR to resolve a few conflicts.

Copy link
Contributor

@arbrandes arbrandes left a comment

Choose a reason for hiding this comment

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

I think that at least for now we're going to go on a different architectural direction. As I've been mentioning elsewhere, the footer (and of course, the header) are perfect candidates for plugin slots: this will allow operators to customize them much more flexibly than just setting a few hard-coded links.

This effort is already in progress via openedx/wg-frontend#178, and we're hoping to make it available in time for the Redwood release.

For the record, there is a competing implementation of the footer as a slot that also allows individual links to be configured via a backend API call. I figure if we're going with this level of granularity at all, that PR is a better approach:

#405

@arbrandes
Copy link
Contributor

arbrandes commented May 15, 2024

Here's a forum thread for us to discuss the general question of plugin slots vs configuration:

https://discuss.openedx.org/t/plugin-slots-vs-configuration/13009

@itsjeyd
Copy link

itsjeyd commented May 31, 2024

@asadali145 @arbrandes It sounds like this PR is no longer needed. Unless you have any objections, we'll close it next week.

CC @mphilbrick211

@asadali145
Copy link
Contributor Author

@arbrandes Thanks for all of the work around the Plugin Slots.

We were waiting for the Redwood release to test the FooterSlot. Here are a few things that we have noticed:

CC: @pdpinch

@arbrandes
Copy link
Contributor

So, as per recent conversations with @brian-smith-tcril and @davidjoy, going forward we're going to be ok with a configuration-controlled header and footer (beyond just plugin slots), provided the configuration mechanism is vetted and consistently applied, which is definitely not the case with the .env variables that exist now.

To summarize a few key consequences of the above:

  • This sane configuration mechanism doesn't yet exist, but is being worked on by @davidjoy as part of the OEP-65/Module Federation work that's currently targetting Teak.
  • We won't accept drive-by PRs adding configuration: each new configuration structure will have to undergo the product proposal and review process, with a proper proposal and justification such as the Header's.

It would be a great benefit to the community if you wrote just such a proposal for the footer. The sooner it gets reviewed and approved, the higher the chance that we can convince David to include some of it as he works on frontend-base (where both Header and Footer will live, going forward).

@asadali145
Copy link
Contributor Author

asadali145 commented Oct 1, 2024

@arbrandes Thanks for the detailed reply. I have written a proposal for the footer https://openedx.atlassian.net/wiki/spaces/OEPM/pages/4512514079/Proposal+Customizable+Footer+for+LMS+CMS. Please add your thoughts on the proposal if any.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
open-source-contribution PR author is not from Axim or 2U product review PR requires product review before merging
Projects
Status: Product Review
Development

Successfully merging this pull request may close these issues.

10 participants