Fix time estimate quick action and avoid negative values for GraphQL mutation to set time estimate
The following discussion from !107892 (merged) should be addressed:
-
@cablett started a discussion: (+3 comments) Is there a reasonable maximum here? Should we add a spec for values like negative numbers (I see !107892 (diffs) we do sort of handle them)?
Thanks for your feedback @gweaver
🙂 Then I'll go ahead preparing this MR to avoid negative values and open up a follow-up to "fix" the quick action🙂 And, "updating" the estimate is already a requested feature in #336780, so we could definitely work on that later on
🙂
Implementation plan
- Edit the quick action code to add a validation for the parsed time estimate provided, to make sure it's not negative
- The code is here: https://gitlab.com/gitlab-org/gitlab/-/blob/d6795b50764e6b85e36789f554e49b24d2f92217/lib/gitlab/quick_actions/issue_and_merge_request_actions.rb#L164
- We can return the error message to the user by setting it to
@execution_message[:estimate]
(see other examples inee/lib/ee/gitlab/quick_actions/epic_actions.rb
) - Update the specs accordingly in https://gitlab.com/gitlab-org/gitlab/-/blob/6619389a921f91382d67b785c40246664c82ab8a/spec/services/notes/quick_actions_service_spec.rb#L134
- Edit the update issue GraphQL mutation to validate the time estimate provided and return an error
- This is done in here: https://gitlab.com/gitlab-org/gitlab/-/blob/d6795b50764e6b85e36789f554e49b24d2f92217/app/graphql/mutations/issues/update.rb#L57
- Update the specs accordingly in https://gitlab.com/gitlab-org/gitlab/-/blob/6619389a921f91382d67b785c40246664c82ab8a/spec/graphql/mutations/issues/update_spec.rb#L164
- Edit the update MR GraphQL mutation in the same way
- This is done in here: https://gitlab.com/gitlab-org/gitlab/-/blob/d6795b50764e6b85e36789f554e49b24d2f92217/app/graphql/mutations/merge_requests/update.rb#L48
- Update the specs accordingly in https://gitlab.com/gitlab-org/gitlab/-/blob/6619389a921f91382d67b785c40246664c82ab8a/spec/graphql/mutations/merge_requests/update_spec.rb#L42
- Update REST API
- Update the code at https://gitlab.com/gitlab-org/gitlab/-/blob/master/lib/api/time_tracking_endpoints.rb#L74 to do the conversion and check for non negative value, in which case it should report an error and don't make any change
- Probably update the list of error codes that can be returned to also contain 400 bad request, but better ask the reviewers for this
- Tweak the API specs adding a test for negative values that should return error: https://gitlab.com/gitlab-org/gitlab/-/blob/master/spec/support/shared_examples/requests/api/time_tracking_shared_examples.rb#L22
Edited by Missy Davies