-
Notifications
You must be signed in to change notification settings - Fork 22
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
fix: Handle guest feature error, no nested catch #2709
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2709 +/- ##
=======================================
Coverage 98.43% 98.43%
=======================================
Files 855 855
Lines 12087 12092 +5
Branches 3163 3181 +18
=======================================
+ Hits 11898 11903 +5
Misses 185 185
Partials 4 4
Continue to review full report in Codecov by Sentry.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found @@ Coverage Diff @@
## main #2709 +/- ##
=======================================
Coverage 98.43% 98.43%
=======================================
Files 855 855
Lines 12087 12092 +5
Branches 3163 3129 -34
=======================================
+ Hits 11898 11903 +5
Misses 185 185
Partials 4 4
Continue to review full report in Codecov by Sentry.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found Additional details and impacted files@@ Coverage Diff @@
## main #2709 +/- ##
=====================================
Coverage 98.44 98.44
=====================================
Files 855 855
Lines 12087 12092 +5
Branches 3163 3164 +1
=====================================
+ Hits 11898 11903 +5
Misses 185 185
Partials 4 4
Continue to review full report in Codecov by Sentry.
|
Bundle ReportChanges will increase total bundle size by 46 bytes ⬆️
|
✅ Deploy preview for gazebo ready!Previews expire after 1 month automatically.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
@@ Coverage Diff @@
## main #2709 +/- ##
=======================================
Coverage 98.43% 98.43%
=======================================
Files 855 855
Lines 12087 12092 +5
Branches 3179 3185 +6
=======================================
+ Hits 11898 11903 +5
Misses 185 185
Partials 4 4
Continue to review full report in Codecov by Sentry.
|
Bundle ReportChanges will increase total bundle size by 46 bytes ⬆️
|
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.
I don't think we should be removing the catch. It looks like it was also there to catch other potential rejections that aren't being handled anymore
src/shared/api/api.js
Outdated
.catch((err) => { | ||
if (err.status === 401 && config.IS_SELF_HOSTED) { | ||
}).then(async (res) => { | ||
const data = await res.json() |
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.
h
: What happens if the call to res.json()
throws? I believe that was also being handled by the catch.
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.
fair enough, we used to have: if res.ok => res.json(), otherwise throw an error, this was recently changed when i added the catch block.
if (data?.errors) { | ||
if ( | ||
data?.errors?.[0]?.extensions?.status === 403 && | ||
config.IS_SELF_HOSTED | ||
) { | ||
window.location.href = '/login' | ||
} | ||
} |
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.
I just want to double confirm here, users who are logged in, but maybe not have access to something let's say the admin panel in self hosted (which users REST endpoints) won't get redirect to the login page right?
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.
No, any exceptions handled in REST views must be derived from BaseException, which is not sent with specific extension codes, only messages (they are all handled as 500 server errors). And for what it's worth, we don’t raise 403 anywhere in the REST API
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.
Sorry I totally missed that this change is only occurring in the graphqil
method 🤦
* fix: Handle guest feature error, no nested catch * Update tests to reflect graphql error * Fix when to raise an error * my bad bro * Update to re add catch block
* feat: Create useRepoComponents hook * We need branch not term * fix: Handle guest feature error, no nested catch (#2709) * fix: Handle guest feature error, no nested catch * Update tests to reflect graphql error * Fix when to raise an error * my bad bro * Update to re add catch block * build: grab github sha from env (#2672) Update Sentry deps and bundler plugin config to prepare for incoming fix to source map uploading. * ref: Convert file entry stuff to ts (#2698) Part of codecov/engineering-team#588 * chore: Remove bundle analysis flags (#2693) Remove bundle analysis feature flags from Gazebo. GH codecov/engineering-team#999 GH codecov/engineering-team#1214 * Prepare release 24.4.1 (#2737) * Update to use react props with children --------- Co-authored-by: nicholas-codecov <nicholas.deschenes@sentry.io> Co-authored-by: Spencer Murray <159931558+spalmurray-codecov@users.noreply.github.com> Co-authored-by: Codecov Releaser <devops+releaser@codecov.io>
Description
/login
on denied access error, following this change feat: Guest feature attempt 2 codecov-api#458Link to Sample Entry
Legal Boilerplate
Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. In 2022 this entity acquired Codecov and as result Sentry is going to need some rights from me in order to utilize my contributions in this PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.