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: handle reactions to comments #8313

Closed
6543 opened this issue Sep 29, 2019 · 20 comments · Fixed by #9220
Closed

API: handle reactions to comments #8313

6543 opened this issue Sep 29, 2019 · 20 comments · Fixed by #9220
Labels
modifies/api This PR adds API routes or modifies them type/enhancement An improvement of existing functionality

Comments

@6543
Copy link
Member

6543 commented Sep 29, 2019

GET     /repos/{owner}/{repo}/issues/comments/{id}/reactions
PUT     /repos/{owner}/{repo}/issues/comments/{id}/reactions
DELETE  /repos/{owner}/{repo}/issues/comments/{id}/reactions
@lunny lunny added modifies/api This PR adds API routes or modifies them type/enhancement An improvement of existing functionality labels Sep 30, 2019
@6543
Copy link
Member Author

6543 commented Nov 2, 2019

GitHub docu https://developer.github.com/v3/issues/comments/#reactions-summary
think we can do better

@6543

This comment has been minimized.

@guillep2k
Copy link
Member

[
  {
    "reaction": "heart",
    "users": [ "6543" ]
  },
  {
    "reaction": "laugh",
    "users": [ "6543", "mmarif" ]
  },
  {
    "reaction": "confused",
    "users": [ "user1" ]
  }
]

Is better: more concise, no ids exposed.

@mmarif4u
Copy link

mmarif4u commented Nov 4, 2019

I agree with @guillep2k, that is the right way to do it. 👍

@6543
Copy link
Member Author

6543 commented Nov 5, 2019

PUT / DELETE use as body

{
    "reaction": "heart",
    "users": [ "6543" ]
 }

I could make it eaven mor simpler by just allow a string to be send or

"reaction": "heart"

but i leave it open so admins have an API to send ractions for other users/bots... too

@zeripath
Copy link
Contributor

zeripath commented Nov 5, 2019

Don't forget Sudo allows an admin to act like any other user - so you should only add that if you're going to allow the admin to set multiple user's reactions.

@lafriks
Copy link
Member

lafriks commented Nov 5, 2019

imho we should follow GitHub API compatibility:
https://developer.github.com/v3/reactions/

Link above is just reaction counts by type in issue details API

@6543
Copy link
Member Author

6543 commented Nov 5, 2019

@lafriks @techknowlogick one downside:

consider a issue with 15 comments -> each had ~ 10 reactions
-> api return user information in the worst case 150 times (dedundancy)
and if there all other users we still load a huge amount of user information wich isnt used ...
and if an app like to load more about a user it still kan get it by its name

I'm thinking about mobile conection ...

@lafriks add more fields beside the user{} can be usefull but should i expose the reaction ID (-> @guillep2k)?

@6543
Copy link
Member Author

6543 commented Nov 5, 2019

proposal2

GET /repos/{owner}/{repo}/issues/comments/{id}/reactions
consume: null
return:

[
  {
    "user": "octocat",
    "content": "heart",
    "created_at": "2016-05-20T20:09:31Z"
  },
  {
    "user": "6543",
    "content": "heart",
    "created_at": "2017-10-20T20:09:31Z"
  }
]

PUT/DELETE /repos/{owner}/{repo}/issues/comments/{id}/reactions
consume:

content: "heart"

return:

[
  {
    "user": "octocat",
    "content": "heart",
    "created_at": "2016-05-20T20:09:31Z"
  }
]

@lafriks
Copy link
Member

lafriks commented Nov 6, 2019

I don't agree as most probably you will still want to have user avatar url, user url, we do return this format in all other api, so I would not like this to be different. Reaction summary (counts by type) you will already have them in issue/pr/comment API. These are details so you really need to request them rarely - when you really want to know exact details

@6543
Copy link
Member Author

6543 commented Nov 7, 2019

@lafriks
GET /repos/{owner}/{repo}/issues/{index} as no reaction info
GET /repos/{owner}/{repo}/pulls/{index} also

a GET /repos/{owner}/{repo}/issues/comments/{id} do NOT exist!
and GET /repos/{owner}/{repo}/issues/comments has no reaction info

this is same at GET /repos/{owner}/{repo}/issues/{index}/comments

@6543
Copy link
Member Author

6543 commented Nov 7, 2019

so add a "reactions_light" or so type wich has smal foodprint to ⬆️ API endpoints?

and make /repos/{owner}/{repo}/issues/comments/{id}/reactions like github?

@6543
Copy link
Member Author

6543 commented Nov 7, 2019

@lafriks github has a similar aproach: https://developer.github.com/v3/issues/#reactions-summary

@6543
Copy link
Member Author

6543 commented Nov 7, 2019

one thing wich is not goog at this reaction_summary_api:
parsing it with custom reactions is not that great - i dont like to hardcode reaction types to a data struct!

@6543
Copy link
Member Author

6543 commented Nov 7, 2019

@lafriks I'm thinking of #8811

@6543
Copy link
Member Author

6543 commented Nov 7, 2019

Proposal.3

reaction endpoint

GET /repos/{owner}/{repo}/issues/comments/{id}/reactions
consume: null
return:

[
  {
    "id": 1,
    "user": {
      "login": "octocat",
      "id": 1,
      "avatar_url": "https://github.com/images/error/octocat_happy.gif",
      "url": "https://api.github.com/users/octocat",
      ..... (gitea_api_user) 
      "type": "User",
      "site_admin": false
    },
    "content": "heart",
    "created_at": "2016-05-20T20:09:31Z"
  }
]

PUT/DELETE /repos/{owner}/{repo}/issues/comments/{id}/reactions
consume:

content: "heart"

return:
http code

Extend issue/pr/comment API

add a reaction field: (api.reaction_summary)

"reactions": [
  {
    "reaction": "heart",
    "users": [ "6543" ],
    "count": 1
  },
  {
    "reaction": "rocket",
    "users": [ "octa" ],
    "count": 1
  }
],
"reactions_count": 2,
"reactions_url": "/repos/{owner}/{repo}/<issues/comments/pull ...>/{id}/reactions"

@lafriks
Copy link
Member

lafriks commented Nov 8, 2019

Extend issue/pr/comment API

Github API for this is better imho

"reactions": {
  "total_count": 5,
  "+1": 3,
  "-1": 1,
  "laugh": 0,
  "confused": 0,
  "heart": 1,
  "hooray": 0,
  "url": "{owner}/{repo}/issues/comments/{id}/reactions"
}

@6543
Copy link
Member Author

6543 commented Nov 8, 2019

@lafriks what if i like to add "rocket" 🚀?
this is no array. types are hardcoded :(
and if we like support for custom reactions we cant provide a usefull summary representation

@6543
Copy link
Member Author

6543 commented Nov 8, 2019

I'll do it the github way, but don't blame me for limitating api afterwards ...

@6543
Copy link
Member Author

6543 commented Dec 2, 2019

Gitea Reactions Summary

"reactions_summary": [
  {
    "reaction": "heart",
    "users": [ "6543" ],
    "count": 1
  },
  {
    "reaction": "rocket",
    "users": [ "octa" ],
    "count": 1
  }
],
"reactions_counter": 2,

EDIT: wont imlement jet - only if somebody need this, if so comment on this issue :D

@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
modifies/api This PR adds API routes or modifies them type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants