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

[API] add endpoint to check notifications [Extend #9488] #9595

Merged
merged 6 commits into from
Jan 14, 2020

Conversation

6543
Copy link
Member

@6543 6543 commented Jan 3, 2020

extend #9488

an endpoint wich e.g. mobile clients can use to check if news are available (use less bandwith than any other notification endpoint ...)

-> GET /notifications/new

  • status 200 (OK) -> Notifications Available
  • status 204 (No Content) -> News Notifications

@codecov-io
Copy link

codecov-io commented Jan 3, 2020

Codecov Report

Merging #9595 into master will increase coverage by 0.03%.
The diff coverage is 80%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9595      +/-   ##
==========================================
+ Coverage    42.3%   42.33%   +0.03%     
==========================================
  Files         599      600       +1     
  Lines       78330    78357      +27     
==========================================
+ Hits        33135    33170      +35     
+ Misses      41140    41129      -11     
- Partials     4055     4058       +3
Impacted Files Coverage Δ
models/issue_comment.go 46.4% <0%> (ø) ⬆️
routers/api/v1/notify/notifications.go 100% <100%> (ø)
routers/api/v1/api.go 74.45% <100%> (+0.03%) ⬆️
models/issue.go 55.42% <100%> (ø) ⬆️
models/notification.go 64.13% <50%> (-0.17%) ⬇️
services/pull/temp_repo.go 31.62% <0%> (-2.57%) ⬇️
models/webhook.go 70.46% <0%> (+1.06%) ⬆️
modules/queue/workerpool.go 41.2% <0%> (+1.28%) ⬆️
services/pull/check.go 53.37% <0%> (+4.72%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ce274d6...c5cb836. Read the comment docs.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jan 3, 2020
@6543 6543 changed the title [API] add endpoint to check notifications [API] add endpoint to check notifications [Extend #9488] Jan 3, 2020
@6543 6543 mentioned this pull request Jan 3, 2020
12 tasks
@6543 6543 force-pushed the add-api-notify-mobile branch 2 times, most recently from 06b824f to 98719ff Compare January 9, 2020 12:10
@lunny lunny added the modifies/api This PR adds API routes or modifies them label Jan 11, 2020
@lafriks lafriks added this to the 1.12.0 milestone Jan 11, 2020
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jan 11, 2020
Copy link
Contributor

@zeripath zeripath left a comment

Choose a reason for hiding this comment

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

Would it be possible to make it return the number of unread notifications or do you think that would be slower?

models/issue_comment.go Outdated Show resolved Hide resolved
models/notification.go Outdated Show resolved Hide resolved
@6543
Copy link
Member Author

6543 commented Jan 12, 2020

Would it be possible to make it return the number of unread notifications or do you think that would be slower?

since github doesnt have souch an endpoint it's gitea-definde :)

@zeripath: I can send a simple json struct new: 2 or so ?

@zeripath
Copy link
Contributor

That would do - my reasoning is that this might be a useful thing for us to query instead of rendering the number of notifications directly on the server.

@6543
Copy link
Member Author

6543 commented Jan 12, 2020

Bildschirmfoto zu 2020-01-12 16-32-57
@zeripath added/changed

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jan 12, 2020
Copy link
Member

@sapk sapk left a comment

Choose a reason for hiding this comment

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

I will not block but I feel that 302 found (even more with json content) is not the best response code compared to what is defined in the RFC. I will try to have a look later if merged.

6.4.3.  302 Found

   The 302 (Found) status code indicates that the target resource
   resides temporarily under a different URI.  Since the redirection
   might be altered on occasion, the client ought to continue to use the
   effective request URI for future requests.
   The server SHOULD generate a Location header field in the response
   containing a URI reference for the different URI.  The user agent MAY
   use the Location field value for automatic redirection.  The server's
   response payload usually contains a short hypertext note with a
   hyperlink to the different URI(s).

      Note: For historical reasons, a user agent MAY change the request
      method from POST to GET for the subsequent request.  If this
      behavior is undesired, the 307 (Temporary Redirect) status code
      can be used instead.

routers/api/v1/notify/notifications.go Outdated Show resolved Hide resolved
templates/swagger/v1_json.tmpl Outdated Show resolved Hide resolved
@6543
Copy link
Member Author

6543 commented Jan 14, 2020

@sapk 302s Name was to good to be true for my intention ... the meaning was different :/ ...

so 200 and 204 are totaly ok 👍

integrations/api_notification_test.go Outdated Show resolved Hide resolved
@sapk
Copy link
Member

sapk commented Jan 14, 2020

CI failed because swagger order the http response and now 200 is on top of 204.

@6543
Copy link
Member Author

6543 commented Jan 14, 2020

I'm now on my PC I'll patch this localy ...

@6543
Copy link
Member Author

6543 commented Jan 14, 2020

@sapk done

@6543
Copy link
Member Author

6543 commented Jan 14, 2020

ready to merge 🚀

@sapk sapk merged commit 44de66b into go-gitea:master Jan 14, 2020
@6543 6543 deleted the add-api-notify-mobile branch January 14, 2020 15:38
Copy link
Member

@sapk sapk left a comment

Choose a reason for hiding this comment

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

Just to make all checks green

@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/api This PR adds API routes or modifies them
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants