Skip to content

Extract milestone update into callback class

Heinrich Lee Yu requested to merge 368503-refactor-issuable-widget-callbacks into master

What does this MR do and why?

Refactors milestone processing so that we handle them in one place for issues, merge requests, and work items.

This introduces the concept of callback classes as discussed in #368503 (closed). The goals for this approach are:

  1. Single source of truth for widget processing

    Right now, we have duplicated code for issues and merge requests. Plus we check permissions differently in the build service and in the create service leading to bugs / inconsistencies.

  2. Improved security

    These callback classes use an allowlist approach vs the denylist that we do in the current issuable services. Before, if we forget to delete a param that the user cannot set, the value will get assigned when we call issuable.assign_attributes. With this new structure, the permissions checks are also located near the main logic so it is easier to verify and reason about.

  3. Make it easier to write widget code

    When we create a new widget that hooks into the issuable service, we don't need to sprinkle code across the different parts of the service.

    Also, I think having a widget base, create, and update service is a bit too much when most of the time they're doing the same thing.

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Related to #368503 (closed)

Edited by Heinrich Lee Yu

Merge request reports