Services for persisting Cycle Analytics group stages

What does this MR do?

This change introduces service objects for creating, updating and finding cycle analytics group stages.

The models and validations have been already merged, so this change only contains the service objects. In an upcoming MR these services will be exposed via an API to the fronted.

• Feature docs: https://about.gitlab.com/product/cycle-analytics/
• Related issue: https://gitlab.com/gitlab-org/gitlab-ee/issues/12196
• Current feature ("in-memory" stages): https://gitlab.com/-/analytics/cycle_analytics

Specs

Cycle Analytics provides 7 default stages to everyone (FOSS feature), these 7 stages should be available on the group level as well, however reporters should be able to create additional, custom stages.

  Defined for a Group, stored in `analytics_cycle_analytics_group_stages` table.

  +---------------------+
  | Stages              |
  |   +-------------+   |
  |   |   Stage A   |   |     
  |   |             |   |     
  |   +-------------+   |     
  |   | Start Event |   |
  |   +-------------+   |     
  |   | End Event   |   |      
  |   +-------------+   |  
  |                     |
  |   +-------------+   |
  |   |   Stage B   |   |
  |   |             |   |
  |   +-------------+   |
  |   | Start Event |   |
  |   +-------------+   |
  |   | End Event   |   |
  |   +-------------+   |
  |                     |
  |   ...               |
  +---------------------+

Requirements (future):

• Ability to change the order of stages.
• Ability to hide a default stage.

To fulfil these requirements, we must also persist the 7 default stages in the database. To keep the record creation at minimum and avoid background migrations, the 7 default stages is going to be persisted as soon as the reporter attempts to make a change to the list of stages (until then, we provide them as in-memory objects):

• Creating a new, customized stage (for the first time).
• Hiding a default stage.

Queries

Note 1: this is the first time we query the stages. We have nothing in the prod DB.

Note 2: We expect that a group will have max 10-15 analytics_cycle_analytics_group_stages records (premium only).

Exists Query

SELECT  1 AS one FROM "analytics_cycle_analytics_group_stages" WHERE "analytics_cycle_analytics_group_stages"."group_id" = 15 AND "analytics_cycle_analytics_group_stages"."custom" = true AND "analytics_cycle_analytics_group_stages"."id" != 3 LIMIT 1;

Uses the index

Limit  (cost=0.14..2.17 rows=1 width=4) (actual time=0.007..0.007 rows=0 loops=1)
   ->  Index Scan using index_analytics_ca_group_stages_on_group_id_and_name on analytics_cycle_analytics_group_stages  (cost=0.14..2.17 rows=1 width=4) (actual time=0.006..0.006 rows=0 loops=1)
         Index Cond: (group_id = 15)
         Filter: (custom AND (id <> 3))
 Planning Time: 0.129 ms
 Execution Time: 0.028 ms

Find by name query

SELECT "analytics_cycle_analytics_group_stages".* FROM "analytics_cycle_analytics_group_stages" WHERE "analytics_cycle_analytics_group_stages"."group_id" = 4 AND "analytics_cycle_analytics_group_stages"."name" = 'issue' LIMIT 1;

Uses the index

Limit  (cost=0.14..2.16 rows=1 width=578) (actual time=0.016..0.016 rows=0 loops=1)
   ->  Index Scan using index_analytics_ca_group_stages_on_group_id_and_name on analytics_cycle_analytics_group_stages  (cost=0.14..2.16 rows=1 width=578) (actual time=0.014..0.015 rows=0 loops=1)
         Index Cond: ((group_id = 4) AND ((name)::text = 'issue'::text))
 Planning Time: 0.163 ms
 Execution Time: 0.042 ms
(5 rows)

Screenshots

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

closes #32570 (closed)

	1 Warning
	This merge request is quite big (more than 629 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	Mark Fletcher (@markglenfletcher)	Thong Kuah (@tkuah)

Generated by Danger

added 1 commit

• dd09c3b3 - Service objects for CA group stages

Compare with previous version

unmarked as a Work In Progress

changed title from WIP: Initial commit to Services for persisting Cycle Analytics group stages

changed the description

changed milestone to %12.4

added devopsfoundations groupoptimize typefeature + 1 deleted label

changed the description

added 1 commit

• f6f54ce3 - Service objects for CA group stages

Compare with previous version

added 1 commit

• 8f09c049 - Service objects for CA group stages

Compare with previous version

added 1 commit

• 85ffba2c - Service objects for CA group stages

Compare with previous version

changed the description

added 1 commit

• 033a5712 - Service objects for CA group stages

Compare with previous version

changed the description

Hi @matteeyah ,

Could you take a look?

Let me know if you need more context.

assigned to @matteeyah

Thank you for tackling this @ahegyi! I did a first round of review. It's mostly about test architecture and code style.

unassigned @matteeyah

resolved all threads

added 154 commits

• 033a5712...a04def94 - 152 commits from branch master
• db66f761 - Service objects for CA group stages
• 7bf4314f - Address CR remarks

Compare with previous version

Thanks for the thorough review @matteeyah, really great suggestions. Pipeline is passing so

assigned to @matteeyah

contexts should define a "set of circumstances" or parameters for an example. Their names usually start with "when ..." (e.g. "when data is invalid") to closer describe the variables state in that context. Specs in this MR are in imperative - i.e. they group a set of examples instead of defining a specific state that is being tested.

Single lined lets should be separated from block bodied lets (let(:variable_name) do ... end) with an empty line.

The implementation looks much cleaner/simpler. Thanks @ahegyi! There's still a couple of spec related things that I left discussions about - the most important one being the organization of states into contexts.

unassigned @matteeyah

added 1 commit

• cb8c5eca - Address CR remarks

Compare with previous version

added 1 commit

• afc96be3 - Address CR remarks

Compare with previous version

assigned to @matteeyah

Generally speaking we usually only have inline example definitions it { is_expected.to ... } or block bodied example definitions it 'example name' do ... end within a single example group (like a context or describe). I think we shouldn't mix them - if we can use the inline form everywhere in the example group great, if not they should all be blocks.

This already looks several times more clean and readable than the initial iteration. Thank you for taking the effort to make sure the code is top notch @ahegyi!

I left a couple more, less important, stylistic spec related discussions. We're getting really close.

unassigned @matteeyah

resolved all threads

resolved all threads

added 1 commit

• f1edd81f - Address CR remarks

Compare with previous version

added 1 commit

• 639b37f2 - Address CR remarks

Compare with previous version

assigned to @matteeyah

I left a minor changelog related discussion. Other than that, this LGTM

Feel free to assign to a maintainer after resolving the changelog discussion.

unassigned @matteeyah

added backstage [DEPRECATED] label

removed typefeature label

added 226 commits

• 639b37f2...b5668b83 - 224 commits from branch master
• a8d8feb2 - Service objects for CA group stages
• 97a9b5fd - Address CR remarks

Compare with previous version

resolved all threads

resolved all threads

Hi @splattael,

Could you do the maintainer review?

assigned to @splattael

unassigned @splattael

Hey @ahegyi

Starting now, I'll be OOO for 3 months (yes, months - Parental Leave).

You might need to choose another maintainer for this MR. Sorry for the friction and I hope all goes well so this MR will be merged timely

added 328 commits

• 97a9b5fd...89d1e617 - 325 commits from branch master
• 452cb174 - Service objects for CA group stages
• 99e6e111 - Address CR remarks
• 98a6c3de - Move stage services to Stages namespace

Compare with previous version

added 1 commit

• 418ee7bf - Move stage services to Stages namespace

Compare with previous version

added 1 commit

• 68347056 - Move stage services to Stages namespace

Compare with previous version

resolved all threads

Hi @jprovaznik,

Could you do the maintainer review?

The initial maintainer review was done by @splattael, however he'll be OOO for a while. I added most of the suggested changes except introducing a Base service class. Reasoning:

• Currently we have 3 services: List, Create, Update.
• The constructor is the same for Create and Update.
• All 3 services have the execute method, however the implementation differs.
• The execute method for Create and Update is similar but not the same. I find it overkill to generalize it (could use template method with calling super), it wouldn't reduce the duplication and would introduce more abstraction.
  • permission check, persisting method, returning http_status are different.

assigned to @jprovaznik

Thanks @ahegyi, overall this looks good ! I left some comments inline, let me know what you think.

(out of scope of this MR): It's little bit hard to review this w/o seeing also the code which would actually use it (API or controllers). In future, when doing this iteratively, it might be easier to split the work not by code layers (first models, then all services then controllers), but rather by functionality (first ability to list default stages, then add ability to create a new one, then update existing...) - these can be hidden behind a feature flag during development.

unassigned @jprovaznik

resolved all threads

added 300 commits

• 68347056...0c3852b1 - 297 commits from branch master
• c9d291be - Service objects for CA group stages
• a0d79002 - Address CR remarks
• 310a8683 - Move stage services to Stages namespace

Compare with previous version

assigned to @jprovaznik

Thanks @ahegyi, left some replies inline, let me know what you think.

unassigned @jprovaznik

added 135 commits

• 310a8683...d8f2eb01 - 132 commits from branch master
• 86fd986e - Service objects for CA group stages
• 613dbd83 - Address CR remarks
• bfa78ebd - Move stage services to Stages namespace

Compare with previous version

added 1 commit

• 3014840b - Move stage services to Stages namespace

Compare with previous version

assigned to @jprovaznik

Thanks @ahegyi, this looks good ! I left one more really minor note in specs, I'm sorry for missing this in previous review rounds. It's just a small optimization.

unassigned @jprovaznik

added 64 commits

• 3014840b...bc246dd0 - 61 commits from branch master
• 0ef46774 - Service objects for CA group stages
• fce4b18e - Address CR remarks
• 73218c2a - Move stage services to Stages namespace

Compare with previous version

assigned to @jprovaznik

approved this merge request

resolved all threads

Thanks @ahegyi, LGTM

merged

mentioned in commit 679c13da

changed the description

added Category:Value Stream Management label