Thinking on the test strategy and I have one question.
Would there also be backend work for this well, in order to tell if a user has ever setup the Slack notifications feature? Or are we basing that logic on if the integration is active or not?
That's a brilliant question, @sgregory2. I would assume that we do not need it to be advanced - as long as the user doesn't use the notifications integrations now we should be enough for us. However, we need this info not on the integration itself but outside (on the integration's table), and I'm not sure whether we have this information at that level. If we do, then no backend is needed. If we don't, then this should be exposed somehow.
I started looking into this and the UX recommendation (found here) is this:
Remove Slack notifications as an option from the integrations list
This is best done on the backend since we both know if a user is on GitLab.com and if they have setup the Slack notifications integration. I guess we can add some sort of condition to disabled_integrations that excludes it when the right condition is met?
I will ask the team if anyone has bandwidth for a relatively small backend issue.
Out of curiosity, @justin_ho why would we need to get all the way to backend if we can make it in the frontend? Why not something as simple as Remove Slack notifications as an option from th... (!108910 - merged)? This way we would be able to deliver as soon as possible and cleanup backend whenever we have time instead of holding back. I need to add some tests to that MR, but we can make another backend issue to clean up after the whole migration is done if it's needed.
What do you think about it? Am I missing something here?
FYI, I have finished the referenced frontend MR and sent it to review. In this regard, I assign the issue to myself for this milestone. As aforementioned, we can discuss changes to backend at a clean up stage
I am not saying it can't be done on the frontend but that it should be done on the backend. If you already have the MR then great we can use that (though I didn't see the GitLab.com check there, not sure if intentional).
IMO the "Rails way" of doing this would be backend since this is purely filtering logic. Not to mention the backend already has some logic for hiding disabled integrations that can be reused.
@justin_ho What do you think about you pushing the backend change? As you mentioned is a relatively small change. I understand your main focus is the frontend, but you can also help with the backend if necessary, and you know about it.
I am not saying it can't be done on the frontend but that it should be done on the backend.
I tend to agree here. If I am understanding correctly, I think handling it from the backend would also allow us to remove the ability to configure and activate the Slack notifications integration from the API as well as the direct edit url. MR comment
I'm not sure why we can not do it from both sides frontend is by far the fastest solution here, and the MR is already in review. If we want a bullet-proof solution for this particular issue (which is just a tiny part of the bigger story), we can do it in parallel, and frontend will simply continue working after that. So I think it would be a win-win.
frontend is by far the fastest solution here, and the MR is already in review
This makes sense I think, if the goal of the epic is to simply provide visual warning about an upcoming deprecation, then frontend makes sense
For actually deprecating slack notifications, maybe it would be good to have a note to remove handling feature flag state from the frontend. My main concern is that it's easy for backend and frontend to become out of sync based on different branches from the same state.
I'm not sure, the best case scenario is unused code that will complicate or confuse troubleshooting efforts.
Juggling multiple branches of truth isn't ideal, even if it works.
I totally support that, @sgregory2. The fewer SOTs we have, the better. However, we should compromise at times and cut corners. Routing everything through backend would be ideal, of course. And, to be frank, it would make the life of frontend easier, of course Nevertheless, the practice of having the flag in both frontend and backend is very common for the product. It allows to speed up the progress and increases velocity for the feature development. Responsibility and communication between BE and FE are the only things to keep in mind.
With that being said, I didn't necessarily mean two separate branches for the flag removal (or global enablement), even though it can be done that way with proper communication. I meant to create an Issue to keep it on the radar and simplify planning/triaging: instead of creating a shared issue with shared weight, we can weigh properly. At the same time, the flag's removal or enablement can indeed come from one consolidated branch. No problem here, I think. It all boils down to communication and coordination.