-
Notifications
You must be signed in to change notification settings - Fork 81
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: Adding human readable 403 error access restricted #1569
Conversation
Thanks for the pull request, @farhaanbukhsh! What's next?Please work through the following steps to get your changes ready for engineering review: 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo 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:
🔘 Get a green buildIf 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 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:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
4343567
to
3c103c9
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1569 +/- ##
==========================================
+ Coverage 92.97% 92.99% +0.01%
==========================================
Files 1075 1078 +3
Lines 21205 21359 +154
Branches 4562 4616 +54
==========================================
+ Hits 19715 19862 +147
- Misses 1424 1425 +1
- Partials 66 72 +6 ☔ View full report in Codecov by Sentry. |
0d905f9
to
bac3bee
Compare
@ChrisChV can you review this? |
bac3bee
to
b403144
Compare
Sure @farhaanbukhsh I will review it tomorrow |
ff9c70b
to
1ba6bb4
Compare
@ChrisChV I have fixed the comments and in addition to that I have fixed a typo that I encountered. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
export const getErrorDetails = (error, dismissible = true) => { | ||
const errorInfo = { dismissible }; | ||
if (error.response?.status === 403) { | ||
// For 403 status the error should be dismissible |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// For 403 status the error should be dismissible | |
// For 403 status the error should not be dismissible |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch :) fixed. Please merge it when the tests go green 😄
Signed-off-by: Farhaan Bukhsh <[email protected]>
Signed-off-by: Farhaan Bukhsh <[email protected]>
1ba6bb4
to
4ccb21c
Compare
Description
Updated to have human readable forbidden error (403), this change came into picture from the discussion
on issue 361
Useful information to include:
New Alert
Supporting information
Discussion on issue 361
Testing instructions
tutor dev launch
tutor mounts add < path to frontend-app-authoring>
tutor images build openedx-dev
tutor dev stop
followed bytutor dev start -d
Deadline
"None" if there's no rush, or provide a specific date or event (and reason) if there is one.Other information
Private ref: BB-9399