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.

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

Add unauthorised200 parameter to /user endpoint #1763

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

paskal
Copy link
Collaborator

@paskal paskal commented May 2, 2024

This resolves #1188 without making extra HTTP calls by adding a new parameter to the /api/v1/user endpoint.

Continuation of accidentally closed #1752.

Copy link

github-actions bot commented May 2, 2024

size-limit report 📦

Path Size
public/embed.mjs 2.03 KB (0%)
public/remark.mjs 73.49 KB (+0.05% 🔺)
public/remark.css 8.26 KB (+0.02% 🔺)
public/last-comments.mjs 35.72 KB (+0.01% 🔺)
public/last-comments.css 3.75 KB (0%)
public/deleteme.mjs 12.11 KB (+0.34% 🔺)
public/counter.mjs 751 B (0%)

Copy link

codecov bot commented May 2, 2024

Codecov Report

Attention: Patch coverage is 41.17647% with 10 lines in your changes are missing coverage. Please review.

Project coverage is 65.06%. Comparing base (877765c) to head (e189877).
Report is 1 commits behind head on master.

Files Patch % Lines
frontend/apps/remark42/app/common/api.ts 14.28% 6 Missing ⚠️
frontend/packages/api/clients/public.ts 60.00% 2 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1763      +/-   ##
==========================================
+ Coverage   61.47%   65.06%   +3.59%     
==========================================
  Files         132      140       +8     
  Lines        2998     3521     +523     
  Branches      715      785      +70     
==========================================
+ Hits         1843     2291     +448     
- Misses       1151     1158       +7     
- Partials        4       72      +68     

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

@paskal paskal force-pushed the paskal/improve_get_user branch from 6ad5bec to 52a9ab7 Compare May 2, 2024 17:41
Copy link
Collaborator

@akellbl4 akellbl4 left a 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 this is a proper solution

@paskal paskal force-pushed the paskal/improve_get_user branch from 52a9ab7 to e189877 Compare May 13, 2024 09:55
@coveralls
Copy link

Pull Request Test Coverage Report for Build 9060885089

Details

  • 15 of 15 (100.0%) changed or added relevant lines in 2 files are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.03%) to 84.899%

Files with Coverage Reduction New Missed Lines %
backend/app/notify/notify.go 2 92.73%
Totals Coverage Status
Change from base Build 9026873922: 0.03%
Covered Lines: 5982
Relevant Lines: 7046

💛 - Coveralls

Copy link

@esev esev left a comment

Choose a reason for hiding this comment

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

I'm interested in seeing a fix for #1188. Just a couple drive-by comments in case they can help move this forward.

@@ -291,10 +291,17 @@ func (s *Rest) routes() chi.Router {
rauth.Use(middleware.Timeout(30 * time.Second))
rauth.Use(tollbooth_chi.LimitHandler(tollbooth.NewLimiter(10, nil)))
rauth.Use(authMiddleware.Auth, matchSiteID, middleware.NoCache, logInfoWithBody)
rauth.Get("/user", s.privRest.userInfoCtrl)
Copy link

Choose a reason for hiding this comment

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

Could this line be moved into the open routes group of handlers above, rather than creating a new group for it?

user := rest.MustGetUserInfo(r)
user, err := rest.GetUserInfo(r)
if err != nil {
if r.URL.Query().Get("unauthorised200") == "true" {
Copy link

Choose a reason for hiding this comment

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

Is the unauthorised200 parameter necessary? It looks like the frontend code will already accept a 200 response with null as the JSON response.

Please correct me if I'm wrong, I'm new here, but I think if null can be returned here, it looks like no frontend changes would be needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Page Experience/Core Web Vitals: Browser errors were logged to the console
4 participants