Protect integration sections for Slack App with a feature flag
What does this MR do and why?
The MR introduces the support for the :integration_slack_app_notifications
feature flag in the frontend and uses it to protect sections in Slack Application integration form while there's an ongoing process of merging the Slack Notifications integration into the Slack Application.
For the reference, here's the flowchart for the conditions taken from #372410 (comment 1127170652):
flowchart TB;
id1["show help HTML + sections"]
id2["show ONLY sections"]
id3["show ONLY help HTML"]
A{"Are we on 'gitlab_slack_application'?"} --> |Yes|B{is FF turned ON?}
B -->|Yes|id1;
B -->|No|id3;
D{Do we have sections?};
A --> |No|D;
D --> |"No"|id3;
D --> |"Yes"|id2
Screenshots or screen recordings
Route | Before | After with the flag OFF | After with the flag ON |
---|---|---|---|
Slack Application without sections | ![]() |
![]() |
![]() |
Slack Application with mocked sections (see patch below) | ![]() |
![]() |
![]() |
Jira | ![]() |
![]() |
![]() |
How to set up and validate locally
- Check that the Slack Application (
/-/settings/integrations/gitlab_slack_application/edit
) and Jira (-/settings/integrations/jira/edit) integrations look fine - Apply the patch to get the dummy sections pushed for the Slack Application integrations:
Patch
diff --git a/ee/app/models/integrations/gitlab_slack_application.rb b/ee/app/models/integrations/gitlab_slack_application.rb
index 08c869eca5c..d570109986e 100644
--- a/ee/app/models/integrations/gitlab_slack_application.rb
+++ b/ee/app/models/integrations/gitlab_slack_application.rb
@@ -1,7 +1,7 @@
# frozen_string_literal: true
module Integrations
- class GitlabSlackApplication < Integration
+ class GitlabSlackApplication < BaseChatNotification
default_value_for :category, 'chat'
has_one :slack_integration, foreign_key: :integration_id
@@ -39,7 +39,51 @@ def self.to_param
end
def fields
- []
+ [
+ {
+ type: 'checkbox',
+ name: 'notify_only_broken_pipelines',
+ section: SECTION_TYPE_CONNECTION,
+ help: 'Do not send notifications for successful pipelines.'
+ }.freeze,
+ {
+ type: 'select',
+ name: 'branches_to_be_notified',
+ section: SECTION_TYPE_CONNECTION,
+ title: s_('Integrations|Branches for which notifications are to be sent'),
+ choices: Integrations::Slack.branch_choices
+ }.freeze,
+ {
+ type: 'text',
+ name: 'labels_to_be_notified',
+ section: SECTION_TYPE_CONNECTION,
+ placeholder: '~backend,~frontend',
+ help: 'Send notifications for issue, merge request, and comment events with the listed labels only. Leave blank to receive notifications for all events.'
+ }.freeze,
+ {
+ type: 'select',
+ section: SECTION_TYPE_CONNECTION,
+ name: 'labels_to_be_notified_behavior',
+ choices: [
+ ['Match any of the labels', MATCH_ANY_LABEL],
+ ['Match all of the labels', MATCH_ALL_LABELS]
+ ]
+ }.freeze
+ ].freeze
+ end
+
+ def sections
+ [
+ {
+ type: SECTION_TYPE_TRIGGER,
+ title: s_('Integrations|Notifications trigger')
+ },
+ {
+ type: SECTION_TYPE_CONNECTION,
+ title: s_('Integrations|Notifications'),
+ description: help
+ }
+ ]
end
def chat_responder
- Re-check the aforementioned integrations - those should look exactly as they used to
- Enable the feature flag in the rails console (
rails c
):Feature.enable(:integration_slack_app_notifications)
- Check the integrations again. The Slack Application one should list the two sections now below the grey "Install Slack app" section (keep in mind that in production the sections should never be shown if the Slack application has not been installed yet - here, we're simply mocking the things). The Jira one should not be changed
MR acceptance checklist
This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.
-
I have evaluated the MR acceptance checklist for this MR.
Related to #375046 (closed) and #372410 (closed)
Merge request reports
Activity
changed milestone to %15.5
added Category:Integrations IntegrationSlack featureenhancement frontend groupintegrations [DEPRECATED] sectiondev typefeature + 1 deleted label
assigned to @dmishunov
Suggested Reviewers (beta)
The individuals below may be good candidates to participate in the review based on various factors.
You can use slash commands in comments to quickly assign
/assign_reviewer @user1
.Suggested Reviewers @ntepluhina
,@jivanvl
,@engwan
,@rymai
,@pslaughter
If you do not believe these suggestions are useful, please apply the label Bad Suggested Reviewer. You can also provide feedback for this feature on this issue:
https://gitlab.com/gitlab-org/gitlab/-/issues/357923
.Automatically generated by Suggested Reviewers Bot - an experimental ML-based recommendation engine created by ~"group::applied ml".
added 256 commits
-
4f612249...b0a19fd0 - 251 commits from branch
master
- 6f25eb3b - Added the integration_slack_app_notifications flag
- 87826859 - Testing the current implementation
- d80a9e54 - Functionality implementation with tests coverage
- 0140510e - Added the utility classes
- 5725b08f - Finalised the sections for Slack App integration
Toggle commit list-
4f612249...b0a19fd0 - 251 commits from branch
- A deleted user
added backend feature flag labels
1 Warning featureaddition and featureenhancement merge requests normally have a documentation change. Consider adding a documentation update or confirming the documentation plan with the Technical Writer counterpart.
For more information, see:
- The Handbook page on merge request types.
- The definition of done documentation.
1 Message CHANGELOG missing: If you want to create a changelog entry for GitLab FOSS, add the
Changelog
trailer to the commit message you want to add to the changelog.If you want to create a changelog entry for GitLab EE, also add the
EE: true
trailer to your commit message.If this merge request doesn't need a CHANGELOG entry, feel free to ignore this message.
Reviewer roulette
Changes that require review have been detected!
Please refer to the table below for assigning reviewers and maintainers suggested by Danger in the specified category:
Category Reviewer Maintainer backend Josianne Hyson ( @jhyson
) (UTC+13, 11 hours ahead of@dmishunov
)Pavel Shutsin ( @pshutsin
) (UTC+2, same timezone as@dmishunov
)frontend Lee Tickett ( @leetickett-gitlab
) (UTC+1, 1 hour behind@dmishunov
)Robert Hunt ( @rob.hunt
) (UTC+1, 1 hour behind@dmishunov
)~"group::integrations" (frontend) No engineer is available for automated assignment, please reach out to the #g_manage_integrations
Slack channel or mention@gitlab-org/manage/integrations
for assistance.Maintainer review is optional for ~"group::integrations" (frontend) To spread load more evenly across eligible reviewers, Danger has picked a candidate for each review slot, based on their timezone. Feel free to override these selections if you think someone else would be better-suited or use the GitLab Review Workload Dashboard to find other available reviewers.
To read more on how to use the reviewer roulette, please take a look at the Engineering workflow and code review guidelines. Please consider assigning a reviewer or maintainer who is a domain expert in the area of the merge request.
Once you've decided who will review this merge request, assign them as a reviewer! Danger does not automatically notify them for you.
If needed, you can retry the
danger-review
job that generated this comment.Generated by
Dangeradded 1 commit
- fae53d72 - Finalised the sections for Slack App integration
marked the checklist item I have evaluated the MR acceptance checklist for this MR. as completed
Bundle size analysis [beta]
This compares changes in bundle size for entry points between the commits 53c3eaa6 and fae53d72
Special assetsEntrypoint / Name Size before Size after Diff Diff in percent average 3.59 MB 3.59 MB - 0.0 % mainChunk 1.96 MB 1.96 MB - 0.0 %
Note: We do not have exact data for 53c3eaa6. So we have used data from: e9eecec6.
The intended commit has no webpack pipeline, so we chose the last commit with one before it.Please look at the full report for more details
Read more about how this report works.
Generated by
DangerAllure report
allure-report-publisher
generated test report!e2e-review-qa:
test report for fae53d72expand test summary
+-----------------------------------------------------------------------------------------+ | suites summary | +------------------------------------+--------+--------+---------+-------+-------+--------+ | | passed | failed | skipped | flaky | total | result | +------------------------------------+--------+--------+---------+-------+-------+--------+ | Verify | 12 | 0 | 1 | 0 | 13 | ✅ | | Create | 28 | 0 | 1 | 0 | 29 | ✅ | | Feature flag handler sanity checks | 9 | 0 | 0 | 0 | 9 | ✅ | | Plan | 47 | 0 | 1 | 0 | 48 | ✅ | | Package | 0 | 0 | 1 | 0 | 1 | ➖ | | Manage | 52 | 0 | 8 | 0 | 60 | ✅ | | Configure | 0 | 0 | 1 | 0 | 1 | ➖ | | Secure | 2 | 0 | 0 | 0 | 2 | ✅ | | Govern | 2 | 0 | 0 | 0 | 2 | ✅ | | Version sanity check | 0 | 0 | 1 | 0 | 1 | ➖ | +------------------------------------+--------+--------+---------+-------+-------+--------+ | Total | 152 | 0 | 14 | 0 | 166 | ✅ | +------------------------------------+--------+--------+---------+-------+-------+--------+
e2e-package-and-test:
test report for fae53d72expand test summary
+---------------------------------------------------------------------------+ | suites summary | +----------------------+--------+--------+---------+-------+-------+--------+ | | passed | failed | skipped | flaky | total | result | +----------------------+--------+--------+---------+-------+-------+--------+ | Create | 320 | 0 | 14 | 0 | 334 | ✅ | | Plan | 114 | 0 | 0 | 0 | 114 | ✅ | | Package | 0 | 0 | 6 | 0 | 6 | ➖ | | Verify | 86 | 0 | 16 | 0 | 102 | ✅ | | Manage | 179 | 1 | 26 | 0 | 206 | ❌ | | Secure | 46 | 0 | 2 | 0 | 48 | ✅ | | Release | 8 | 0 | 0 | 0 | 8 | ✅ | | Fulfillment | 4 | 0 | 28 | 0 | 32 | ✅ | | Configure | 0 | 0 | 6 | 0 | 6 | ➖ | | Analytics | 4 | 0 | 0 | 0 | 4 | ✅ | | Version sanity check | 0 | 0 | 2 | 0 | 2 | ➖ | | Govern | 4 | 0 | 0 | 0 | 4 | ✅ | +----------------------+--------+--------+---------+-------+-------+--------+ | Total | 765 | 1 | 100 | 0 | 866 | ❌ | +----------------------+--------+--------+---------+-------+-------+--------+
- Resolved by Enrique Alcántara
I believe we do not need a backend review here as it's a pure frontend MR. Hence I would like to ask @slashmanov for a technical review and @joseph for a domain expert review in the course of #372410 (closed)
requested review from @slashmanov and @joseph
mentioned in issue #372410 (closed)
@joseph
, thanks for approving this merge request.This is the first time the merge request is approved. To ensure full test coverage, a new pipeline will be started shortly.
For more info, please refer to the following links:
removed review request for @joseph
mentioned in issue #377527 (closed)
requested review from @ealcantara
enabled an automatic merge when the pipeline for 0b70e9aa succeeds
mentioned in commit 5c30ab80
added workflowstaging-canary label
added workflowcanary label and removed workflowstaging-canary label
added workflowstaging label and removed workflowcanary label
added workflowproduction label and removed workflowstaging label
added releasedcandidate label
1 --- 2 name: integration_slack_app_notifications 3 introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/98663 4 rollout_issue_url: @dmishunov We are missing the feature flag rollout issue. Can you create the issue or tell me if we need to use the
Feature Flag Cleanup
or theFeature Flag Roll Out
template, please? When should we expect the removal of the feature flag?Will take care of creating the rollout issue next week, @arturoherrero. Thanks for the ping. The flag is supposed to be rolled out once we have the migration of all Slack Notifications into the Slack Application integration. Once that's done, we can roll out the flag. Presumably, it should be safe before or during the "Release Pre-requisites" phase as listed in &8670 (closed)
@arturoherrero, here's the rollout issue: #381012 (closed).
@dmishunov Thanks! Please review !102620 (merged)
added devopsmanage label and removed 1 deleted label
added releasedpublished label and removed releasedcandidate label
added groupimport and integrate label and removed groupintegrations [DEPRECATED] label