Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Contents view: Item selection persists over different locations #5209

Closed
mbarde opened this issue Sep 21, 2023 · 5 comments · Fixed by #6554
Closed

Contents view: Item selection persists over different locations #5209

mbarde opened this issue Sep 21, 2023 · 5 comments · Fixed by #6554

Comments

@mbarde
Copy link

mbarde commented Sep 21, 2023

Describe the bug

Selections made in contents view persist even if user navigates to different location. This can lead to dangerously unexpected behavior (for example accidentally deleting the wrong items).

To Reproduce
Steps to reproduce the behavior:

  1. Go to contents view
  2. Select item
  3. Navigate into a sub folder
  4. Select another item
  5. Click delete icon
  6. Even currently not visible selected items will be deleted. Furthermore the delete confirmation modal does not show the titles of those selected items (see Screenshot).

Expected behavior

Only try to delete the currently visible selected items.

Or at least display all titles of items correctly that are going to be deleted.

Screenshots

Screenshot_2023-09-21_12-22-37

Software

  • Volto 16.23.0
  • Plone 6.0.6
  • plone.restapi 8.40.0
  • OS: Ubuntu
  • Browser Firefox, Chrome
@raghavKasyap
Copy link

The reason this happens is because of the fact that this.state.selected stores only the ids of the items which have been selected. And any logic that involves retrieving information corresponding to the selected elements is done by getting the item's info from this.state.items which is populated.
This state.items is updated on travelling along the content structure of the application, however state.selected still persists which raises cases where the item that is selected is not present in this.state.items and hence would leave us with dangling selected ids whose information is not available.

Solution Proposed.

I think the best solution for this is to store the entire information of the items that are selected rather than just storing the ids in this.state.selected.

Please do help me with any other approaches to this problem.

@raghavKasyap
Copy link

raghavKasyap commented Nov 20, 2023

Here is an other place where this error occurs.
image

The root cause being the same.

@mbarde
Copy link
Author

mbarde commented Dec 1, 2023

Another proposal:

In componentDidUpdate of Contents.jsx add this:

if (
      this.state.items !== prevState.items &&
      this.state.selected.length > 0
    ) {
      this.setState({
        selected: this.state.selected.filter((id) =>
          find(this.state.items, { '@id': id }),
        ),
      });
    }

As soon as the items the component has access to change, we want to update selected to ensure only the items stay in the selection which the component can actually access.
Fixes described bugs & leads to more consistent behavior (imho) since only the items which are currently visible to the user stay in selection.

@raghavKasyap
Copy link

raghavKasyap commented Dec 7, 2023

That's eventually re-setting selected Items to empty whenever we navigate to a new location right. Since whenever we go to a new location none of the previously selected items are going to be present in the new this.state.items (because we have come to a new location).
What I effectively mean to say is that
this.state.selected.filter((id) => find(this.state.items, { '@id': id }))
will always return empty array. (correct me if I am wrong here)
That's honestly a good way to solve this issue as that would prevent any inconsistent behaviour and users don't have to remember what they have selected before.

@mbarde
Copy link
Author

mbarde commented Dec 12, 2023

Yes, I think you are right.

My proposed code snippet should ensure that in selected state only items persist which do actually exist in this.state.items to fix above described display bugs.

But yes, I think in fact this will always set selected to an empty array, which I agree to be the expected behavior.
So we could shorten it to:

if (
      this.state.items !== prevState.items &&
      this.state.selected.length > 0
    ) {
      this.setState({
        selected: [],
      });
    }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants