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

[WIP] [API] add ReactionSummary to Commit and Issue #9497

Closed
wants to merge 10 commits into from

Conversation

6543
Copy link
Member

@6543 6543 commented Dec 25, 2019

rev: #8313 (comment)

reaction_summary is nil or:

"reaction_summary": { 
      "heart": 1,
      "laugh": 2,
      "rocket": 6
}
  • function itself

  • exposed for issues

    • add test
  • exposed for issue_comments (should but not working

    • add test
  • dont return user -> only count emojy by type

@6543 6543 changed the title [API] add ReactionSummary to Commit and Issue [WIP] [API] add ReactionSummary to Commit and Issue Dec 25, 2019
@6543 6543 force-pushed the api-add-reaction-summary branch from 055142d to e0fd483 Compare December 25, 2019 22:13
@6543 6543 force-pushed the api-add-reaction-summary branch from e0fd483 to 1113784 Compare December 25, 2019 22:18
@codecov-io
Copy link

codecov-io commented Dec 25, 2019

Codecov Report

Merging #9497 into master will decrease coverage by 0.00%.
The diff coverage is 94.44%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9497      +/-   ##
==========================================
- Coverage   42.60%   42.59%   -0.01%     
==========================================
  Files         673      673              
  Lines       73862    73877      +15     
==========================================
+ Hits        31470    31471       +1     
- Misses      37275    37313      +38     
+ Partials     5117     5093      -24     
Impacted Files Coverage Δ
modules/structs/issue.go 0.00% <ø> (ø)
modules/convert/issue.go 86.04% <87.50%> (-1.26%) ⬇️
models/issue.go 56.83% <100.00%> (+0.24%) ⬆️
models/issue_comment.go 47.78% <100.00%> (-5.66%) ⬇️
models/issue_reaction.go 83.15% <100.00%> (+0.76%) ⬆️
modules/util/timer.go 42.85% <0.00%> (-42.86%) ⬇️
services/mailer/mail.go 54.83% <0.00%> (-1.08%) ⬇️
modules/notification/mail/mail.go 34.48% <0.00%> (ø)
services/pull/pull.go 41.27% <0.00%> (+0.49%) ⬆️
... and 9 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 6f27849...81cd91a. Read the comment docs.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 25, 2019
@lafriks
Copy link
Member

lafriks commented Dec 25, 2019

I still think it should same as github with only counts:

"reactions": {
  "total_count": 5,
  "+1": 3,
  "-1": 1,
  "laugh": 0,
  "confused": 0,
  "heart": 1,
  "hooray": 0,
  "url": "<api>/repos/octocat/Hello-World/issues/comments/1/reactions"
}

@6543
Copy link
Member Author

6543 commented Dec 26, 2019

@lafriks the problem is that github's responce don't allow custom reaction

should i rename the field from reaction_summary to gitea_reactions or something else to make sure it wont colide with github one?

@6543 6543 changed the title [WIP] [API] add ReactionSummary to Commit and Issue [API] add ReactionSummary to Commit and Issue Dec 28, 2019
@6543 6543 changed the title [API] add ReactionSummary to Commit and Issue [WIP] [API] add ReactionSummary to Commit and Issue Dec 28, 2019
@lafriks
Copy link
Member

lafriks commented Dec 29, 2019

Why wouldn't it allow custom reactions? Pretty much it would not allow only reactions with name url and total_count

@6543
Copy link
Member Author

6543 commented Dec 29, 2019

dont think this is a nice format to parce! @lafriks "reactions": is no array or list!

as far as I understand it's a section for defined fields ... so you cant simply itterate throu :(

@lafriks
Copy link
Member

lafriks commented Dec 29, 2019

Also I think that returning all users is too verbose, let's say there will be 1000 likes for issue or comment, for summary we should return only counts and if users are needed than we already have api for that

@lafriks
Copy link
Member

lafriks commented Dec 29, 2019

It's dictionary ;)

@6543
Copy link
Member Author

6543 commented Dec 29, 2019

@lafriks this is why i only use usernames but i can make it smaler ... it should realy be a summary.

the api to get detailed reaction is well defined and done at #9220

-> I'll remove the user and only return counter of reactions

@lafriks
Copy link
Member

lafriks commented Dec 29, 2019

https://swagger.io/docs/specification/data-models/dictionaries/

@6543
Copy link
Member Author

6543 commented Dec 29, 2019

@lafriks update #9497 (comment)

@lunny lunny added the modifies/api This PR adds API routes or modifies them label Jan 25, 2020
@stale
Copy link

stale bot commented Mar 24, 2020

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs during the next 2 months. Thank you for your contributions.

@stale stale bot added the issue/stale label Mar 24, 2020
@6543
Copy link
Member Author

6543 commented Mar 31, 2020

no stale - it was just not enouth time to talk about how the responce should look like
Just have to finish it

@stale stale bot removed the issue/stale label Mar 31, 2020
@lunny lunny added the issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented label Apr 1, 2020
@6543 6543 closed this Mar 1, 2022
@6543 6543 deleted the api-add-reaction-summary branch March 1, 2022 02:56
@go-gitea go-gitea locked and limited conversation to collaborators Apr 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. modifies/api This PR adds API routes or modifies them
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants