fix: convert notification banner to text if accounts url is not set #362

asadali145 · 2024-10-02T09:33:11Z

Description

The notification banner is always visible and cannot be dismissed. If ACCOUNT_SETTINGS_URL is not set, it redirects to <ora_grading>/notifications, which is not a valid URL.

This PR displays text instead of a link when the ACCOUNT_SETTINGS_URL is not set.

Screenshots

Before:

After (If ACCOUNT_SETTINGS_URL is not set):

After (If ACCOUNT_SETTINGS_URL is set):

openedx-webhooks · 2024-10-02T09:33:16Z

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 maintained by @openedx/openedx-unmaintained. Tag them in a comment and let them know that your changes are ready for review.

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.

codecov · 2024-10-02T09:48:14Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (91e3374) to head (b014d97).
Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff            @@
##            master      #362   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          111       111           
  Lines         1082      1088    +6     
  Branches       159       165    +6     
=========================================
+ Hits          1082      1088    +6

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

src/containers/NotificationsBanner/index.jsx

brian-smith-tcril · 2024-10-17T14:30:33Z

src/containers/NotificationsBanner/index.jsx

-      </Hyperlink>
+      {
+        isEmpty(getConfig().ACCOUNT_SETTINGS_URL) ? (
+          <FormattedMessage {...messages.notificationsBannerLinkMessage} />


It feels a little odd to me to have the notificationsBannerLinkMessage only be a link sometimes. I think it makes more sense to have 2 (technically 3) fully independent strings:

infoMessage + notificationsBannerLinkMessage when we have an ACCOUNT_SETTINGS_URL

Something else entirely when we do not have an ACCOUNT_SETTINGS_URL

Another option that may be simpler would just be renaming notificationsBannerLinkMessage to notificationsBannerPreferencesCenterMessage.

@sarina which option we choose feels a little like a product question to me. Should I tag someone to get a product opinion? If not I'd be happy to merge this with the rename to notificationsBannerPreferencesCenterMessage as that feels like a smaller change.

well for i18n you should embed the URL within the phrase, not concat them, if that's what you're suggesting.

I think the easiest is to rename the message. I think there's a separate DEPR for removing this banner altogether.

Sounds good to me!

@asadali145 - if notificationsBannerLinkMessage is renamed to notificationsBannerPreferencesCenterMessage this PR will look ready to merge to me!

@brian-smith-tcril thanks for the review. PR is ready.

brian-smith-tcril · 2024-10-23T17:21:25Z

src/containers/NotificationsBanner/index.jsx

@@ -1,4 +1,5 @@
 import React from 'react';
+import { isEmpty } from 'lodash';


Do we know all the possible "empty" values that getConfig().ACCOUNT_SETTINGS_URL returns? I'd prefer we avoid adding a lodash dependency to this file if possible.

Yeah, I have replaced the lodash check with the values check. Could you please have another look?

Also, Just a reminder that we have another CTA banner removal PR that's pending for review #347.

brian-smith-tcril

Thank you for updating the logic not not require lodash!

asadali145 · 2024-11-04T14:13:37Z

@brian-smith-tcril Could you please merge this?

brian-smith-tcril · 2024-11-04T16:40:40Z

@asadali145 it looks like this PR needs a rebase as branch protections require it not be out-of-date with the base branch. Would you be able to update it? Once that's done I can hit the merge button.

…rPreferencesCenterMessage

asadali145 · 2024-11-04T16:50:57Z

@brian-smith-tcril Thank you, I just rebased and the checks are passing.

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Oct 2, 2024

asadali145 changed the title ~~feat: make notification banner dismissible~~ fix: convert notification banner to text if accounts url is not set Oct 3, 2024

asadali145 force-pushed the asad/update-notifications-banner branch from 8d3709e to 011c6fe Compare October 3, 2024 15:36

asadali145 requested a review from awais-ansari October 3, 2024 15:37

asadali145 mentioned this pull request Oct 7, 2024

feat: remove CTA #347

Merged

asadali145 requested a review from brian-smith-tcril October 7, 2024 14:02

awais-ansari requested changes Oct 9, 2024

View reviewed changes

src/containers/NotificationsBanner/index.jsx Outdated Show resolved Hide resolved

asadali145 requested a review from awais-ansari October 14, 2024 08:52

awais-ansari approved these changes Oct 14, 2024

View reviewed changes

brian-smith-tcril reviewed Oct 17, 2024

View reviewed changes

brian-smith-tcril reviewed Oct 23, 2024

View reviewed changes

asadali145 requested a review from brian-smith-tcril October 28, 2024 10:55

brian-smith-tcril approved these changes Nov 1, 2024

View reviewed changes

asadali145 added 5 commits November 4, 2024 21:46

fix: convert banner to text if ACCOUNT_SETTINGS_URL is not set

0f83d77

refactor: refactoring

2b2efe2

refactor: rename notificationsBannerLinkMessage to notificationsBanne…

da0b998

…rPreferencesCenterMessage

refactor: remove lodash usage

4e9039e

refactor: remove lodash usage

b014d97

asadali145 force-pushed the asad/update-notifications-banner branch from a448045 to b014d97 Compare November 4, 2024 16:46

brian-smith-tcril merged commit c9d0abe into openedx:master Nov 4, 2024
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: convert notification banner to text if accounts url is not set #362

fix: convert notification banner to text if accounts url is not set #362

asadali145 commented Oct 2, 2024 •

edited

Loading

openedx-webhooks commented Oct 2, 2024

codecov bot commented Oct 2, 2024 •

edited

Loading

brian-smith-tcril Oct 17, 2024

sarina Oct 17, 2024

brian-smith-tcril Oct 17, 2024

asadali145 Oct 18, 2024

brian-smith-tcril Oct 23, 2024

asadali145 Oct 28, 2024

brian-smith-tcril left a comment

asadali145 commented Nov 4, 2024

brian-smith-tcril commented Nov 4, 2024

asadali145 commented Nov 4, 2024

		@@ -1,4 +1,5 @@
		import React from 'react';
		import { isEmpty } from 'lodash';

fix: convert notification banner to text if accounts url is not set #362

fix: convert notification banner to text if accounts url is not set #362

Conversation

asadali145 commented Oct 2, 2024 • edited Loading

Description

Screenshots

openedx-webhooks commented Oct 2, 2024

What's next?

🔘 Get product approval

🔘 Provide context

🔘 Get a green build

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

Who will review my changes?

Where can I find more information?

When can I expect my changes to be merged?

codecov bot commented Oct 2, 2024 • edited Loading

Codecov Report

brian-smith-tcril Oct 17, 2024

Choose a reason for hiding this comment

sarina Oct 17, 2024

Choose a reason for hiding this comment

brian-smith-tcril Oct 17, 2024

Choose a reason for hiding this comment

asadali145 Oct 18, 2024

Choose a reason for hiding this comment

brian-smith-tcril Oct 23, 2024

Choose a reason for hiding this comment

asadali145 Oct 28, 2024

Choose a reason for hiding this comment

brian-smith-tcril left a comment

Choose a reason for hiding this comment

asadali145 commented Nov 4, 2024

brian-smith-tcril commented Nov 4, 2024

asadali145 commented Nov 4, 2024

asadali145 commented Oct 2, 2024 •

edited

Loading

codecov bot commented Oct 2, 2024 •

edited

Loading