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.

Sign up for GitHub

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

Jump to bottom

fix: wrong input argument in ErrorBanner #288

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

fix: wrong input argument in ErrorBanner #288

wants to merge 10 commits into from

Conversation

FatemeKhodayari
Copy link

@FatemeKhodayari FatemeKhodayari commented Nov 12, 2023 •
edited
Loading

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

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

Additional info

More info can be found in this topic

Closes 285

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

openedx-webhooks commented Nov 12, 2023 •
edited
Loading

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.

@mphilbrick211 mphilbrick211 added the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Nov 13, 2023
@e0d e0d removed the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Nov 20, 2023
@e0d
Copy link
Contributor

e0d commented Nov 20, 2023

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

@FatemeKhodayari
Copy link
Author

@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.

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

codecov bot commented Dec 14, 2023 •
edited
Loading

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.

@e0d e0d removed the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Dec 14, 2023
@mphilbrick211 mphilbrick211 added the waiting for eng review PR is ready for review. Review and merge it, or suggest changes. label Dec 14, 2023
@mphilbrick211 mphilbrick211 requested a review from a team December 14, 2023 22:07
@mphilbrick211
Copy link

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

@FatemeKhodayari
Copy link
Author

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
Copy link

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!

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

FatemeKhodayari commented Feb 13, 2024 •
edited
Loading

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.

@e0d e0d removed the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Feb 26, 2024
@arbrandes arbrandes self-requested a review March 14, 2024 14:06
@feanil feanil removed the request for review from a team March 20, 2024 15:09
arbrandes
arbrandes requested changes Mar 20, 2024
View reviewed changes
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.

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?

@FatemeKhodayari
Copy link
Author

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
Copy link

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

@mphilbrick211
Copy link

Friendly ping on this, @arbrandes!

@mphilbrick211
Copy link

Friendly ping on this, @arbrandes!

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arbrandes arbrandes Awaiting requested review from arbrandes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
open-source-contribution PR author is not from Axim or 2U waiting for eng review PR is ready for review. Review and merge it, or suggest changes.
Projects
Contributions
Status: In Eng Review
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ORA grading not displaying ORAs with file uploads
5 participants
@FatemeKhodayari @openedx-webhooks @e0d @mphilbrick211 @arbrandes