Resolve "Mark as viewed stays checked in Single File Mode"
What does this MR do?
For #299948 (closed)
Switches to using the Vuex as the source of file reviews, not relying on a single fetch out of localStorage when the component loads.
This fixes numerous issues with the "Viewed" checkbox on files when it's used in the single-file view mode.
MR | Description |
---|---|
We are here |
Because we don't always render a new Diff File component, incorporating file reviews with a "pull" method doesn't work, we need a "push" method a la Vuex subscriptions. |
Future MR | We don't need to store reviews nested under their file_identifier_hash , since each ID incorporates the file and the MR, we'll simplify the review storage by just storing the review information associated to the file ID at the top level. |
Screenshots (strongly suggested)
For these scenarios, we trigger a full page refresh, then perform the recorded actions.
Scenario | Notes | Before | After |
---|---|---|---|
"Typical", check the viewed box, then move to the next (unviewed) file | Here note that in the before, the check persisted when moving to the next file, but the collapsed state didn't | beforeTypical | afterTypical |
Where the initial load is of a "Viewed" file, and we move to a file that is not marked as viewed | Here note that it has the same behavior as the previous scenario - the check incorrectly persists to the next file | beforeViewedToNot | afterViewedToNot |
Where the initial load is of a file that is not "Viewed", and we move to a file that is viewed | Here, note that neither the check nor the collapsed state updates properly when moving between files with different states | beforeNotToViewed | afterNotToViewed |
Where the initial load is of a "Viewed" file, then we uncheck it, and move to the next - not viewed - file | Note here that the uncheck incorrectly flows into the next file, which should be checked (and collapsed) | beforeUncheckViewedToViewed | afterUncheckViewedToViewed |
Does this MR meet the acceptance criteria?
Conformity
-
📋 Does this MR need a changelog?-
I have included a changelog entry. - [-] I have not included a changelog entry because _____.
-
- [-] Documentation (if required)
-
Code review guidelines -
Merge request performance guidelines -
Style guides - [-] Database guides
- [-] Separation of EE specific content
Availability and Testing
-
Review and add/update tests for this feature/bug. Consider all test levels. See the Test Planning Process. -
Tested in all supported browsers - [-] Informed Infrastructure department of a default or new setting change, if applicable per definition of done
Security
If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:
- [-] Label as security and @ mention
@gitlab-com/gl-security/appsec
- [-] The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
- [-] Security reports checked/validated by a reviewer from the AppSec team
Edited by Thomas Randolph