Skip to content

Conversation

@dcoa
Copy link
Contributor

@dcoa dcoa commented Oct 13, 2025

Create a custom error boundary for libraries team management workflow

This PR creates a custom error boundary that maps errors 401/403/404/5xx

Changes overview

  • Add a env variable to performs redirections to authoring MFE libraries
  • Create a reload button for 5xx error that will retry the query
  • Create a custom error to display error access denied when the user do not have permissions over a library (the validation permission endpoint does not performs that)

Evidence

image image

@openedx-webhooks openedx-webhooks added open-source-contribution PR author is not from Axim or 2U core contributor PR author is a Core Contributor (who may or may not have write access to this repo). labels Oct 13, 2025
@openedx-webhooks
Copy link

Thanks for the pull request, @dcoa!

This repository is currently maintained by @openedx/committers-frontend.

Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are 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.


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.

@github-project-automation github-project-automation bot moved this to Needs Triage in Contributions Oct 13, 2025
@dcoa dcoa changed the title feat(authz): create an custom error boundery feat(authz): [FC-0099] create an custom error boundery Oct 13, 2025
@codecov
Copy link

codecov bot commented Oct 13, 2025

Codecov Report

❌ Patch coverage is 94.33962% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.44%. Comparing base (315e5c6) to head (03fcc19).
⚠️ Report is 7 commits behind head on master.

Files with missing lines Patch % Lines
src/index.tsx 0.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master      #11      +/-   ##
==========================================
+ Coverage   92.29%   92.44%   +0.15%     
==========================================
  Files          31       33       +2     
  Lines         454      503      +49     
  Branches       77       80       +3     
==========================================
+ Hits          419      465      +46     
- Misses         34       37       +3     
  Partials        1        1              

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@mphilbrick211 mphilbrick211 added the FC Relates to an Axim Funded Contribution project label Oct 15, 2025
@mphilbrick211 mphilbrick211 moved this from Needs Triage to Ready for Review in Contributions Oct 15, 2025
Copy link

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

Looks good, though I did have one nit.

resetErrorBoundary();
};
return (
<Container className="d-flex flex-column align-items-center justify-content-center min-vh-100 bg-light-200" data-testid="error-page">

Choose a reason for hiding this comment

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

Suggested change
<Container className="d-flex flex-column align-items-center justify-content-center min-vh-100 bg-light-200" data-testid="error-page">
<Container className="d-flex flex-column align-items-center justify-content-center min-vh-100 bg-light-200">

Do you foresee a need to use getByTestId? If not, I'd rather we only add it if necessary (for the usual reasons).

Copy link
Contributor

@jacobo-dominguez-wgu jacobo-dominguez-wgu left a comment

Choose a reason for hiding this comment

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

I have not tested on my local yet, but overall it looks good. I have added a couple of comments with some suggestions let me know your thoughts. Nice work!

</ErrorBoundary>,
);
const reloadBtn = screen.getByText(/Reload Page/i);
fireEvent.click(reloadBtn);
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be interesting to use userEvent instead of fireEvent to handle user interactions since it simulates a full user interaction, rather than just a single event, leading to more realistic and robust tests https://testing-library.com/docs/user-event/intro/

});
};

const librariesUrl = () => `${getConfig().AUTHORING_BASE_URL}/authoring/libraries`;
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not getting why this one is a function and not just a constant.

Suggested change
const librariesUrl = () => `${getConfig().AUTHORING_BASE_URL}/authoring/libraries`;
const LIBRARIES_URL = `${getConfig().AUTHORING_BASE_URL}/authoring/libraries`;

And maybe it can be moved to the constants file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason why is a function is because as a constant the code return undefined

image

Because this is the only component that uses the url I left it here, I will put the variable directly in the destination prop instead, like this https://github.com/openedx/frontend-app-account/blob/c7bbe8d0d1f08e00595bcb6de4f032b2c8c1d6bb/src/id-verification/panels/SummaryPanel.jsx#L54

} = getErrorConfig({ errorMessage, errorStatus });

const handleReload = () => {
if (reloading) { return; }
Copy link
Contributor

Choose a reason for hiding this comment

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

This line maybe not needed since the button is disabled when reloading is true.

@dcoa dcoa force-pushed the dcoa/error-boundery branch 2 times, most recently from 30bdcf7 to 670aa28 Compare October 20, 2025 02:30
@dcoa
Copy link
Contributor Author

dcoa commented Oct 20, 2025

@jacobo-dominguez-wgu @arbrandes I addressed your comments, and I just change the env variable to COURSE_AUTHORING_MICROFRONTEND_URL to avoid duplicate values in tutor-mfe for example.

@dcoa dcoa requested a review from arbrandes October 20, 2025 02:33
@dcoa
Copy link
Contributor Author

dcoa commented Oct 20, 2025

@jacobo-dominguez-wgu
Copy link
Contributor

@dcoa Thanks for addressing my suggestions. I tested it on the sandbox you provided and this is what I found

  • The 403 page loaded correctly:
Captura de pantalla 2025-10-21 a la(s) 1 07 00 p m image

@dcoa
Copy link
Contributor Author

dcoa commented Oct 22, 2025

The reason why that is happening is that the authZ API that is the initial evaluation (if I have permission or not) is not validating if a scope exist or not, instead it returns 400 if you put something random because the scope is not properly formatted (I can treat this ones as not found from the front), or not allowed permissions if the format is correct but the library (scope) does not exist.

I manually typed https://apps.singapore.edunext.cloud/admin-console/authz/libraries/lalala in the url to get an error message screen, and I got the next one, my question is if is that expected? should it be the 404 page? Also the back to libraries button took me to https://apps.singapore.edunext.cloud/authoring/authoring/libraries, not sure if it is a env variables matter.

Yes, there is a issue about how the env was defined in the sandbox

@dcoa dcoa force-pushed the dcoa/error-boundery branch from 670aa28 to 03fcc19 Compare October 22, 2025 08:01
@MaferMazu MaferMazu moved this to Ready for testing in RBAC AuthZ Board Oct 22, 2025
@arbrandes arbrandes merged commit 2e1dc75 into openedx:master Oct 23, 2025
6 checks passed
@github-project-automation github-project-automation bot moved this from Ready for review to Done in RBAC AuthZ Board Oct 23, 2025
@github-project-automation github-project-automation bot moved this from Ready for Review to Done in Contributions Oct 23, 2025
@arbrandes arbrandes mentioned this pull request Oct 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core contributor PR author is a Core Contributor (who may or may not have write access to this repo). FC Relates to an Axim Funded Contribution project open-source-contribution PR author is not from Axim or 2U

Projects

Status: Done
Status: Done

Development

Successfully merging this pull request may close these issues.

5 participants