The setting for enabling Visual Reviews is removed due to low usage. There is no replacement feature.
Deprecation Summary
Deprecating Visual Review Tools, with removal in 17.0. The remaining components of Review Apps is not affected by this change. The setting for enabling Visual Reviews will be removed in 17.0 and customers will not longer have access to this feature. We recommend customers stop using this feature prior to 17.0, as it will no longer be supported.
Breaking Change
Affected Topology
This deprecation affects both self-managed and SaaS users.
Affected Tier
Premium
Checklists
remove visual review tool feature
remove the visual-review-bot user
Labels
This issue is labeled deprecation, and with the relevant ~devops::, ~group::, and ~Category: labels.
As soon as possible, but no later than the third milestone preceding the major release (for example, given the following release schedule: 14.8, 14.9, 14.10, 15.0 – 14.8 is the third milestone preceding the major release):
Documentation has been updated to mark the feature as deprecated.
On or before the major milestone: A removal entry has been created so the removal will appear on the removals by milestones page and be announced in the release post.
Your stage's stable counterparts have been @mentioned on this issue. For example, Customer Support, Customer Success (Technical Account Manager), Product Marketing Manager.
To see who the stable counterparts are for a product team visit product categories
If there is no stable counterpart listed for Sales/CS please mention @timtams
If there is no stable counterpart listed for Support please mention @gitlab-com/support/managers
If there is no stable counterpart listed for Marketing please mention @cfoster3
Your GPM has been @mentioned so that they are aware of planned deprecations. The goal is to have reviews happen at least two releases before the final removal of the feature or introduction of a breaking change.
Jocelyn Eillismarked the checklist item This issue is labeled deprecation, and with the relevant ~devops::, ~group::, and ~Category: labels. as completed
marked the checklist item This issue is labeled deprecation, and with the relevant ~devops::, ~group::, and ~Category: labels. as completed
We are keeping this in grouppipeline security as we were the previous owners of Visual Reviews and are likely the best candidates to remove the code at this point in time.
Is there any explanation as to why this is being removed or if some alternative will be made available?
Having the ability to make notes within a deployed review app that show up in a merge or something is a seemingly great feature; I'd love to use it but there's not much sense in using something that's deprecated.. :(
@forbiddenera there has been low usage of the feature up until this point. The maintenance costs outweigh the amount of usage to warrant keeping it.
/cc @jreporter@rutshah
I'm going to point this at a 2 from a backend perspective:
It's currently behind the feature flag anonymous_visual_review_feedback.
From the backend, things to remove:
API::VisualReviewDiscussions
Notes::CreateVisualReviewService
Remove the Docsdoc/ci/review_apps/index.md
Remove the FF push: EE::Projects::MergeRequestsController
Remove visual_review_bot and the 24 references to it. (The feature creates a note with a visiual review bot user). This may require a data migration to completely remove the user which is why I've pointed it at 2.
We could also create a new issue as a follow-up to remove the bot, if we are time crunched during the major milestone. Leaving the bot won't effect the users experience of the feature.
@allison.browne I'm deleting a bunch of the code now, and wondering what will happen to existing Visual Review comments if we delete the review bot. The presence AR validation is enough to make me punt on this for %17.0, but I think we need to come up with a plan for the old comments to decide how to prioritize removing the actual bot user.
@drew, when we remove users we would typically migrate all the comments to the ghost user. That's what happens when users are removed via the UI. Probably if we used the Service that the UI hits to remove users it would do this for us automatically. In any case I think it's fine to punt to another release.
It doesn't look like that flag is being used client side. But it is getting pushed to the FE here ee/app/controllers/ee/projects/merge_requests_controller.rb
I noticed some code added to the MR objectvisualReviewAppAvailable that checks for the feature, but then that isn't being used anywhere either
I do see some code in app/assets/javascripts/vue_merge_request_widget/components/mr_widget_pipeline_container.vue and app/assets/javascripts/vue_merge_request_widget/components/deployment/deployment_list.vue but it doesn't look those props/data are used either.
I think the bulk of the code in-use is dealing with environments and some in the MR widget
@shampton we are revisiting this issue. I see Allison identified a FF for the BE anonymous_visual_review_feedback but I'm not seeing any ties to it on the FE.
Do you remember if you put any of this code behind a FF?
@pburdette no, that feature flag is only on the backend.
Most of the code for the visual review tool is in a separate repository that is installed as an NPM package. If someone tries to submit feedback anonymously without the feature flag enabled, the frontend will receive a 403 response code and tell the user to enable the feature flag.
To remove this feature, it should just be:
Remove the package from the package.json
Remove any reference to it on the frontend, which I believe should just be instructions on how to use it
Remove the backend anonymous feedback endpoint
Hopefully I'm not missing anything, but let me know if you have any other questions and I'm happy to help.
I just learned that this is in a separate repo and we should just need to remove the dependency. No need to FF removal on the client, we can always just re-install the package if the deprecation goes bad for some reason.
Ok, thanks @pburdette and @shampton. @jreporter@rutshah given the change in understanding of complexity, I'm moving this to %17.0. It doesn't sound like there is really anything to do ahead of time.
Remove any reference to it on the frontend, which I believe should just be instructions on how to use it
Remove the backend anonymous feedback endpoint
Removing the FE pieces are the breaking change that has to go in %17.0; do we want to leave the flag off and schedule the backend removal for %17.1? Or remove it all in the same release?
The backend is a straightforward deletion that I'd weight as a 1 if it were split out on its own. But being a quick removal, I'm happy to keep just this one issue and do it all at once.