fix(contentpicker): correctly clear old item when maxSelectable=1 and parent reloaded by cgrdavies · Pull Request #1706 · box/box-ui-elements

cgrdavies · 2019-11-05T17:35:47Z

This change fixes #1705, which was preventing the prior selection from being properly cleared when the content picker was in single file selection mode and the folder enclosing the previously selected item had been reloaded before changing the selection.

Problem

Because the content picker relied on a mutating a reference to the previously selected item, when the items were reloaded from the server, that reference was no longer valid and the item within currentCollection was not updated.

Solution

Scan the currentCollection for an item that matches the prior selection based on the item's id. If found, remove the selected keyword from that item directly.

boxcla · 2019-11-05T17:35:56Z

Hi @cgrdavies, thanks for the pull request. Before we can merge it, we need you to sign our Contributor License Agreement. You can do so electronically here: http://opensource.box.com/cla

Once you have signed, just add a comment to this pull request saying, "CLA signed". Thanks!

cgrdavies · 2019-11-05T17:36:56Z

CLA signed

priyajeet · 2019-11-05T18:22:47Z

+                const priorSelection = selected[priorSelectionIndex];
+                const matchingItem = items.find(potentialMatch => potentialMatch.id === priorSelection.id);
+
+                if (matchingItem) delete matchingItem.selected;


if (matchingItem) {
...
}

priyajeet · 2019-11-05T18:57:56Z

BTW there is a possibility this issue applies to multi-selection. Trying to see when it broke, likely when we switched to paging.

priyajeet · 2019-11-05T19:11:34Z

In order to properly fix this, and frankly make it less confusing, we may have to get rid of the reference mutations and keep selected isolated from currentCollection.items. I am trying a few things, will report back.

priyajeet · 2019-11-05T19:52:51Z

Roughly something along these lines might be better https://github.com/box/box-ui-elements/pull/1709/files. We still can use a map to prevent multiple find()s so that its a O(1) lookup or we can just keep the find()s as in practice selected will always be a small-ish array. Do you want to update your PR following this appraoch?

cgrdavies · 2019-11-06T16:01:48Z

Roughly something along these lines might be better https://github.com/box/box-ui-elements/pull/1709/files. We still can use a map to prevent multiple find()s so that its a O(1) lookup or we can just keep the find()s as in practice selected will always be a small-ish array. Do you want to update your PR following this appraoch?

@priyajeet Thanks for taking a look at this! I agree that removing the reference mutations is a better approach, but I was originally hesitant to do so given the magnitude of the change that would be required. With your outline though I'm happy to update this PR.

…ction-bug

cgrdavies · 2019-11-07T20:02:18Z

@priyajeet: should be all set for another look on this!

priyajeet

lgtm mostly, some minor stylistic comments. Thanks for pulling this in.

priyajeet · 2019-11-07T21:18:38Z

+ * @author Box
+ */
+
+export function containsItem(item: BoxItem) {


Can you move these exports to the bottom. It's our new convention. So

function foo() {} function bar() {} export { foo, bar };

Also if you have time can you add a couple of unit tests for this file for these new functions

priyajeet · 2019-11-07T21:28:24Z

+    return (selected: BoxItem) => selected.id === id && selected.type === type;
+}
+
+export function isSelected(item: BoxItem, selectedItems: Array<BoxItem>) {


For both of these functions probably should add a return type, so : boolean here and : (item: BoxItem) => boolean above

priyajeet · 2019-11-07T21:28:50Z

-    const { name = '', selected = false } = rowData;
+    const { name = '' } = rowData;
    const Component = isRadio ? RadioButton : Checkbox;
+    const isSelected = !!isRowSelected(rowData, selected);


!! is probably unnecessary since the function returns a boolean

priyajeet · 2019-11-07T21:30:35Z

-        const { selected, type } = items[index];
-        const isSelectable = isRowSelectable(selectableType, extensionsWhitelist, hasHitSelectionLimit, items[index]);
+        const { type } = items[index];
+        const isSelected = !!isRowSelected(items[index], selected);


!! not needed here either

priyajeet · 2019-11-07T21:31:05Z

        index: number,
        rowData: BoxItem,
    }) => {
+        const isSelected = !!isRowSelected(rowData, selected);


priyajeet · 2019-11-07T21:40:55Z

-            const cacheKey = this.api.getAPI(type).getCacheKey(id);
            // if shared link already exists, update the collection in state
            this.updateItemInCollection(item);
-            if (item.selected && item !== selected[cacheKey]) {


Actually removing this part may cause a problem, since the item object in question has changed, it needs to be the one that is returned, so one will have to change the choose() function to return items from the updated collection and not whats inside selected.

Further simplification would be to make selected array just an array of IDs and Types, since thats what is really used everywhere to check if a row is selected or not. So that when choose button is pressed, then we build the returned array with latest item objects.

Alternatively, updateItemInCollection() function can be changed to also update selected array with the updated item.

priyajeet · 2019-11-07T21:41:28Z

BTW, if you dont have time to make more changes, we can take over the PR...

…le-select-multiple-selection-bug

cgrdavies · 2019-11-14T14:59:55Z

@priyajeet should be all set from a code perspective. I changed updateItemInCollection to replace the matching item in the selected array.

I do have a failing test though that seems unrelated, and I can't immediately figure out what's going on. Any chance someone could take a look at that? I don't have enough overall context to be able to fix it expediently.

CLAassistant · 2020-03-03T19:05:30Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

sahiga · 2020-03-31T17:57:07Z

Hey @cgrdavies, sorry we've taken so long to merge this PR. I'm looking into the failing tests right now. Is it okay with you if I add commits to your PR? Also, could you sign the new CLA? Thanks!

cgrdavies requested review from a team as code owners November 5, 2019 17:35

priyajeet reviewed Nov 5, 2019

View reviewed changes

cgrdavies added 3 commits November 7, 2019 11:19

fix(content-picker): fix issues; see if tests pass

663eb81

fix(content-picker): factor out commonalities

b913950

Merge branch 'master' of github.com:box/box-ui-elements into fix-sele…

4c32a45

…ction-bug

cgrdavies force-pushed the fix-single-select-multiple-selection-bug branch from 6b4c481 to 4c32a45 Compare November 7, 2019 19:46

fix(content-picker): fix linting in checkboxcellrenderer

4e67b38

priyajeet reviewed Nov 7, 2019

View reviewed changes

cgrdavies added 2 commits November 12, 2019 15:43

Merge branch 'master' of github.com:box/box-ui-elements into fix-sing…

5d5ed70

…le-select-multiple-selection-bug

fix(content-picker): Pr Feedback

5a28b52

cgrdavies force-pushed the fix-single-select-multiple-selection-bug branch from f82b52d to 5a28b52 Compare November 13, 2019 19:20

sahiga added do-not-merge and removed do-not-merge labels Mar 19, 2021

Conversation

cgrdavies commented Nov 5, 2019

Problem

Solution

Uh oh!

boxcla commented Nov 5, 2019

Uh oh!

cgrdavies commented Nov 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

priyajeet commented Nov 5, 2019

Uh oh!

priyajeet commented Nov 5, 2019

Uh oh!

priyajeet commented Nov 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cgrdavies commented Nov 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cgrdavies commented Nov 7, 2019

Uh oh!

priyajeet left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

priyajeet commented Nov 7, 2019

Uh oh!

cgrdavies commented Nov 14, 2019

Uh oh!

CLAassistant commented Mar 3, 2020

Uh oh!

sahiga commented Mar 31, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

cgrdavies commented Nov 5, 2019 •

edited

Loading

priyajeet commented Nov 5, 2019 •

edited

Loading

cgrdavies commented Nov 6, 2019 •

edited

Loading