GraphQL mutations for add, remove and toggle emoji
What does this MR do?
GraphQL mutations for add, remove and toggle emoji
Adding new AddAwardEmoji
, RemoveAwardEmoji
and ToggleAwardEmoji
GraphQL mutations.
Does this MR meet the acceptance criteria?
Conformity
-
Changelog entry - [-] Documentation created/updated or follow-up review issue created
-
Code review guidelines -
Merge request performance guidelines -
Style guides - [-] Database guides
- [-] Separation of EE specific content
Performance and testing
Merge request reports
Activity
changed milestone to %12.1
added Category:Design Management Create [DEPRECATED] Deliverable GraphQL backend devopscreate workflowin dev + 1 deleted label
1 Warning This merge request is quite big (more than 751 lines changed), please consider splitting it into multiple merge requests. Reviewer roulette
Changes that require review have been detected! A merge request is normally reviewed by both a reviewer and a maintainer in its primary category (e.g. frontend or backend), and by a maintainer in all other categories.
To spread load more evenly across eligible reviewers, Danger has randomly picked a candidate for each review slot. Feel free to override this selection if you think someone else would be better-suited, or the chosen person is unavailable.
Once you've decided who will review this merge request, mention them as you normally would! Danger does not (yet?) automatically notify them for you.
Category Reviewer Maintainer backend Heinrich Lee Yu ( @engwan
)James Lopez ( @jameslopez
)Generated by
DangerEdited by 🤖 GitLab Bot 🤖added 1 commit
- c378b64d - GraphQL mutations for add, remove and toggle emoji
marked the checklist item Changelog entry as completed
added 1 commit
- f8142782 - GraphQL mutations for add, remove and toggle emoji
added 1 commit
- e6abe65f - GraphQL mutations for add, remove and toggle emoji
- Resolved by Luke Duncalfe
- Resolved by Luke Duncalfe
- Resolved by Luke Duncalfe
- Resolved by Luke Duncalfe
@reprazent I hope you don't mind if I ask you to also review this MR? It's particularly because I'm interested to know your opinion on this https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/29919#note_184446765. The thinking is that mutation errors could be presented the same as when we raise an error due to authorization checks. The exception turns into:
{ errors: 'Some error' }
Rather than how
mergeRequestWip
currently presents its error:{ data: { errors: 'Some error' } }
The benefit would be the front-end would only need to check for errors in one structure. If the mutation was dealing with a collection and was allowed to mutate some in the collection but not the records that were invalid, it could probably in that case respond with the errors in its normal
data.errors
response. Edit: See https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/29919#note_184446765Edited by Luke Duncalfeassigned to @reprazent
Note: This MR mentions in many places that it will leverage service classes in https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/29782. For now, to allow this to be merged ahead of that MR it will be introducing some ~"technical debt" to be cleaned up when the service classes are available.
- Resolved by Luke Duncalfe
- Resolved by Luke Duncalfe
- Resolved by Luke Duncalfe
- Resolved by Luke Duncalfe
- Resolved by Luke Duncalfe
- Resolved by Luke Duncalfe
- Resolved by Luke Duncalfe
- Resolved by Luke Duncalfe
- Resolved by Bob Van Landuyt
Thanks @.luke, left some thoughts.
Should we merge this after https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/29782 or do you plan to finish this one first and extending https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/29782 to include this?
unassigned @reprazent
added 545 commits
-
e6abe65f...9e6c3f56 - 544 commits from branch
master
- f6a6f551 - GraphQL mutations for add, remove and toggle emoji
-
e6abe65f...9e6c3f56 - 544 commits from branch
- Resolved by Luke Duncalfe
added 1 commit
- 725923f9 - GraphQL mutations for add, remove and toggle emoji
added 1 commit
- 2ff04615 - GraphQL mutations for add, remove and toggle emoji
@reprazent Thank you for your review! I've updated this MR with your feedback. I agree that the
pre_check
is leaky of information about the resource. I haven't yet changed that behaviour because I was wondering whether it's important to have a "nice message" when the resource isn't Awardable https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/29919#note_184888751.assigned to @reprazent
- Resolved by Bob Van Landuyt
- Resolved by Luke Duncalfe
- Resolved by Bob Van Landuyt
- Resolved by Bob Van Landuyt
unassigned @.luke
assigned to @.luke and unassigned @reprazent
added 81 commits
-
2ff04615...87b468c2 - 80 commits from branch
master
- 40736036 - GraphQL mutations for add, remove and toggle emoji
-
2ff04615...87b468c2 - 80 commits from branch
assigned to @reprazent
added 1 commit
- 1ad5887b - GraphQL mutations for add, remove and toggle emoji
- Resolved by Luke Duncalfe
- Resolved by Luke Duncalfe
- Resolved by Luke Duncalfe
- Resolved by Luke Duncalfe
@.luke Replied and some nitpicks. I think moving some stuff out into a new issue is a good idea, but some of those we'd need to address sooner rather than later as they're required for having fully functional notes on designs: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/29919#note_185598593.
Apart from the nitpicks, I think this is ready to move forward, WDYT?
unassigned @reprazent
mentioned in issue #63797 (moved)
mentioned in issue #63798 (moved)
added 1 commit
- 97ff00fe - GraphQL mutations for add, remove and toggle emoji
assigned to @jprovaznik