fix: wrong input argument in ErrorBanner #288

FatemeKhodayari · 2023-11-12T12:40:11Z

When opening an ORA with file uploads, the MFE tries to preview the uploaded file. If anything goes wrong, it displays the error banner.

...
  return (
    <FileCard key={file.downloadUrl} file={file}>
      {isLoading && <LoadingBanner />}
      {errorStatus ? (
        <ErrorBanner {...error} />
      ) : (
        <Renderer {...rendererProps} />
      )}
    </FileCard>
  );
...

As we see in the code, error is passed to ErrorBanner for displaying the error banner component. The ErrorBanner component has an input argument named headingMessage

...
export const ErrorBanner = ({ actions, headerMessage, children }) => {
...

The problem happens here. The error object passed from the FileRenderer component has no headingMessage attribute. This will crash the frontend as it tries to get an id out of the headingMessage to display the translated error message. The attribute available in error is headerMessage. Renaming the input argument will fix this issue.

Test Plan

To reproduce the error

Head to studio and create an ORA with file submissions
Open the ORA in LMS. Upload a file and submit the ORA
At the bottom of the unit in LMS, click on Grade available responses
Log the error object in FileRenderer component for testing purpose or make some change that crashes the FileRenderer component
Click on View all responses button

Additional info

More info can be found in this topic

Closes 285

openedx-webhooks · 2023-11-12T12:40:15Z

Thanks for the pull request, @FatemeKhodayari!

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.

e0d · 2023-11-20T18:07:58Z

@FatemeKhodayari looks like there are some check failing, can you have a look?

FatemeKhodayari · 2023-11-21T10:53:45Z

@FatemeKhodayari looks like there are some check failing, can you have a look?

Sorry. I had forgotten to change the test file as well. Fixed it.

codecov · 2023-12-14T19:36:30Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (9cfab58) to head (fda46a8).

❗ Current head fda46a8 differs from pull request most recent head bbb6d39. Consider uploading reports for the commit bbb6d39 to get more accurate results

Additional details and impacted files

@@            Coverage Diff            @@
##            master      #288   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          109       109           
  Lines         1079      1078    -1     
  Branches       160       159    -1     
=========================================
- Hits          1079      1078    -1

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

mphilbrick211 · 2023-12-14T22:07:36Z

Hi @openedx/content-aurora - this is ready for review. Thanks!

…ument

FatemeKhodayari · 2024-02-12T10:35:45Z

Hi @openedx/content-aurora - this is ready for review. Thanks!

Dear @mphilbrick211, do you have any idea about the state of this PR? It's been open for so long that now it has conflict with the master branch :)) Shall I resolve the conflict so you can proceed with the review or maybe there's a problem with this PR and thus it won't be merged?

mphilbrick211 · 2024-02-12T19:13:23Z

Hi @FatemeKhodayari! I apologize for the delay. I believe the Aurora group is no longer reviewing pull requests, so I will look into another reviewer for you. Ok to hold off on the conflict until we have a reviewer!

FatemeKhodayari · 2024-02-13T07:33:23Z

Hi @FatemeKhodayari! I apologize for the delay. I believe the Aurora group is no longer reviewing pull requests, so I will look into another reviewer for you. Ok to hold off on the conflict until we have a reviewer!

Hi @mphilbrick211! No worries :) Thanks for looking into this. I'll wait to hear back from you before proceeding with the conflict.

…hub.com:FatemeKhodayari/frontend-app-ora-grading into FatemeKhodayari/fix-error-banner-input-argument

…ument

arbrandes

Every other instance of error handling in this MFE uses headingMessage for similar purposes. I believe we should stick to that pattern.

With that in mind, do you mind changing https://github.com/openedx/frontend-app-ora-grading/blob/master/src/components/FilePreview/hooks.js#L82 instead?

…anner args

…ument

…hub.com:FatemeKhodayari/frontend-app-ora-grading into FatemeKhodayari/fix-error-banner-input-argument

FatemeKhodayari · 2024-03-21T11:15:47Z

Every other instance of error handling in this MFE uses headingMessage for similar purposes. I believe we should stick to that pattern.

With that in mind, do you mind changing https://github.com/openedx/frontend-app-ora-grading/blob/master/src/components/FilePreview/hooks.js#L82 instead?

Thanks Adolfo. Applied the requested changes. Sorry for the messy git history by the way. Didn't squash / rebase the commits as I was afraid it may cause more conflict / complexity. Hope you can squash them while merging.

mphilbrick211 · 2024-03-21T21:16:42Z

@arbrandes - would you mind re-enabling the tests to run?

…ument

mphilbrick211 · 2024-05-01T21:07:25Z

Friendly ping on this, @arbrandes!

mphilbrick211 · 2024-07-30T20:45:01Z

Friendly ping on this, @arbrandes!

@arbrandes flagging for when you're able to take a look.

CodeWithEmad · 2024-11-04T07:33:02Z

We can close this in favor of 2208737

fix: wrong input argument in ErrorBanner

ee47235

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

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

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

fix: change headingMessage to headerMessage in ErrorBanner test

b414c3e

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

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

mphilbrick211 added the waiting for eng review PR is ready for review. Review and merge it, or suggest changes. label Dec 14, 2023

mphilbrick211 requested a review from a team December 14, 2023 22:07

Merge branch 'master' into FatemeKhodayari/fix-error-banner-input-arg…

b1dac50

…ument

mphilbrick211 added the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Feb 12, 2024

e0d removed the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Feb 26, 2024

arbrandes self-requested a review March 14, 2024 14:06

FatemeKhodayari and others added 3 commits March 16, 2024 13:11

Merge master into FatemeKhodayari/fix-error-banner-input-argument

2b3f80e

Merge branch 'FatemeKhodayari/fix-error-banner-input-argument' of git…

d38d9d7

…hub.com:FatemeKhodayari/frontend-app-ora-grading into FatemeKhodayari/fix-error-banner-input-argument

Merge branch 'master' into FatemeKhodayari/fix-error-banner-input-arg…

fda46a8

…ument

feanil removed the request for review from a team March 20, 2024 15:09

arbrandes requested changes Mar 20, 2024

View reviewed changes

FatemeKhodayari added 3 commits March 21, 2024 14:29

fix: rename headerMessage to headingMessage so that it matches ErrorB…

69b1173

…anner args

Merge branch 'master' into FatemeKhodayari/fix-error-banner-input-arg…

276d8ee

…ument

Merge branch 'FatemeKhodayari/fix-error-banner-input-argument' of git…

b4b307a

…hub.com:FatemeKhodayari/frontend-app-ora-grading into FatemeKhodayari/fix-error-banner-input-argument

Merge branch 'master' into FatemeKhodayari/fix-error-banner-input-arg…

bbb6d39

…ument

FatemeKhodayari requested a review from arbrandes April 12, 2024 11:45

CodeWithEmad closed this Nov 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: wrong input argument in ErrorBanner #288

fix: wrong input argument in ErrorBanner #288

FatemeKhodayari commented Nov 12, 2023 •

edited

Loading

openedx-webhooks commented Nov 12, 2023 •

edited

Loading

e0d commented Nov 20, 2023

FatemeKhodayari commented Nov 21, 2023

codecov bot commented Dec 14, 2023 •

edited

Loading

mphilbrick211 commented Dec 14, 2023

FatemeKhodayari commented Feb 12, 2024

mphilbrick211 commented Feb 12, 2024

FatemeKhodayari commented Feb 13, 2024 •

edited

Loading

arbrandes left a comment

FatemeKhodayari commented Mar 21, 2024

mphilbrick211 commented Mar 21, 2024

mphilbrick211 commented May 1, 2024

mphilbrick211 commented Jul 30, 2024

CodeWithEmad commented Nov 4, 2024

fix: wrong input argument in ErrorBanner #288

fix: wrong input argument in ErrorBanner #288

Conversation

FatemeKhodayari commented Nov 12, 2023 • edited Loading

Test Plan

Additional info

openedx-webhooks commented Nov 12, 2023 • edited Loading

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?

e0d commented Nov 20, 2023

FatemeKhodayari commented Nov 21, 2023

codecov bot commented Dec 14, 2023 • edited Loading

Codecov Report

mphilbrick211 commented Dec 14, 2023

FatemeKhodayari commented Feb 12, 2024

mphilbrick211 commented Feb 12, 2024

FatemeKhodayari commented Feb 13, 2024 • edited Loading

arbrandes left a comment

Choose a reason for hiding this comment

FatemeKhodayari commented Mar 21, 2024

mphilbrick211 commented Mar 21, 2024

mphilbrick211 commented May 1, 2024

mphilbrick211 commented Jul 30, 2024

CodeWithEmad commented Nov 4, 2024

FatemeKhodayari commented Nov 12, 2023 •

edited

Loading

openedx-webhooks commented Nov 12, 2023 •

edited

Loading

codecov bot commented Dec 14, 2023 •

edited

Loading

FatemeKhodayari commented Feb 13, 2024 •

edited

Loading