Bad UX in Merge Request Security Reports when reports aren't available and backend logic is used
Summary
The Security Report section of the MR widget shows confusing error messages with the dependency_scanning_merge_request_report_api
feature flag enabled, when there is no dependency_scanning
report.
The root cause for this means that this bug will also apply to SAST, DAST and container scanning reports when their respective feature flags are enabled as well.
Steps to reproduce
- Enable the
dependency_scanning_merge_request_report_api
feature flag - Visit a merge request for a project which either:
- has least one other security report configured, but not
dependency_scanning
- has
dependency_scanning
enabled, where the job hasn't finished running yet
- has least one other security report configured, but not
- See that the dependency scanning report row is displayed in an error kind of state
Example Project
gitlab-org/security-products/analyzers/bandit!14 (merged) (although now the feature flag has been disabled on production as a result of this bug)
What is the current bug behavior?
The dependency scanning report section is displayed with an error message Loading resulted in an error
, and the Security Report as a whole will display the error message errors when loading results
.
What is the expected correct behavior?
When there is no dependency_scanning
report (i.e., when the dependency_scanning
job hasn't been set up, or hasn't finished running yet), the dependency scanning report section should not be displayed.
Relevant logs and/or screenshots
Feature flag disabled (Good) | Feature flag enabled (Bad) |
---|---|
Possible fixes
The frontend work for &1425 (closed) changed the display logic for the security report sections (but not the security report row itself, which will eventually manifest as another bug, once all reports' feature flags are enabled).
With the feature flag disabled, the dependency scanning report row's visibility is completely determined by the truthiness of gl.mrWidgetData.dependency_scanning.head_path
(e.g., /gitlab-org/security-reports-no-dep-scanning/-/jobs/1270/artifacts/download?file_type=dependency_scanning&proxy=true
). That path, as I understand it, is only provided if the dependency_scanning
job itself has successfully run. Therefore, before the Vue app even bootstraps, the information about whether to display the dependency scanning report is available.
When the feature flag is enabled, the diffEndpoint
is set in the store, via gl.mrWidgetData.dependency_scanning_comparison_path
, which is always provided, whether or not the job has finished run, or is even set up at all.
So, the simplest fix would be to revert the display logic for the security report sections to only depend on the presence of the head path. However, will that check continue to be valid once https://gitlab.com/gitlab-org/gitlab-ee/issues/12487#note_212760037 lands? I suspect it won't be. Therefore, we'll need to pass to the frontend a set of flags which indicate whether or not to display the appropriate report sections.
Why not just depend on the response from the diffEndpoint
, in that case? Well, that means we'd have to always show all security report rows as loading, even when no security reports have been set up. Once the endpoint responds (e.g., with {"status_reason":"This merge request does not have dependency scanning reports"}
), the relevant row could be hidden. But that's bad UX (why display something as loading, only to remove it once it's loaded). Or, we could hide the security reports sections until one or more diffEndpoint
s have responded; but that again is bad UX (the UI will jump around once the security reports load, after some amount of time).
Given we're planning to replace this whole lot, we probably should use the simplest fix.
If I had more time, I could have written a more concise/clearer issue. For now, this will have to do.