Skip to content

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

Closed
6543 wants to merge 10 commits into
go-gitea:mainfrom
6543-forks:api-add-reaction-summary
Closed

[WIP] [API] add ReactionSummary to Commit and Issue#9497
6543 wants to merge 10 commits into
go-gitea:mainfrom
6543-forks:api-add-reaction-summary

Conversation

@6543

@6543 6543 commented Dec 25, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

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

codecov-io commented Dec 25, 2019 •
edited
Loading

Copy link
Copy Markdown

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

lafriks commented Dec 25, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

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

6543 commented Dec 26, 2019

Copy link
Copy Markdown
Member Author

@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

lafriks commented Dec 29, 2019

Copy link
Copy Markdown
Member

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

@6543

6543 commented Dec 29, 2019 •
edited
Loading

Copy link
Copy Markdown
Member Author

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

lafriks commented Dec 29, 2019

Copy link
Copy Markdown
Member

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

lafriks commented Dec 29, 2019

Copy link
Copy Markdown
Member

It's dictionary ;)

@6543

6543 commented Dec 29, 2019

Copy link
Copy Markdown
Member Author

@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

lafriks commented Dec 29, 2019

Copy link
Copy Markdown
Member

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

@6543

6543 commented Dec 29, 2019

Copy link
Copy Markdown
Member Author

@lafriks update #9497 (comment)

@6543 6543 mentioned this pull request Dec 30, 2019
Closed
@lunny lunny added the kind/api label Jan 25, 2020
@stale

stale Bot commented Mar 24, 2020

Copy link
Copy Markdown

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

6543 commented Mar 31, 2020 •
edited
Loading

Copy link
Copy Markdown
Member Author

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 mentioned this pull request May 30, 2020
Closed
7 tasks
@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.

Reviewers

No reviews

Assignees

No one assigned

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.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@6543 @codecov-io @lafriks @lunny @GiteaBot