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

🪟 🐛 Fixes BulkEditServiceProvider to use filtered streams instead of all streams #21902

Merged

Conversation

YatsukBogdan1
Copy link
Contributor

What

Closes #21551

How

The solution is pretty straightforward, I just used filtered streams to use inside <BulkEditServiceProvider />
https://www.loom.com/share/b02731e1759948e1b299a6ed9836ea3c

But there is a tricky moment here. What should we do in case we checked all streams with some keywords and got for example 50 streams selected, but after that, we typed more symbols to search string and got only 10 results? In this case, those 50 streams will be still selected inside useBulkEditService while only 10 will be visible to the user
https://www.loom.com/share/466809e75bb74157915b91a6494560b0

As you can see right now selectedBatchNodeIds and selectedBatchNodes are not the same in the current implementation. So what should we do with this?
Screenshot 2023-01-26 at 09 50 03

const allChecked = selectedBatchNodes.size === nodes.length;
...
const ctx: BulkEditServiceContext = {
    ...
    allChecked,
    selectedBatchNodeIds: Array.from(selectedBatchNodes).filter((node): node is string => node !== undefined),
    selectedBatchNodes: nodes.filter((n) => selectedBatchNodes.has(n.id)),
    ...
};

@octavia-squidington-iv octavia-squidington-iv added the area/frontend Related to the Airbyte webapp label Jan 26, 2023
@edmundito
Copy link
Contributor

Found a bug while testing:

  1. Filter the streams and select "all"
  2. Bulk edit and hit apply
  3. Unselect the streams

You will notice that there is only the streams that were filtered, but the others are gone!

@dizel852
Copy link
Contributor

What should we do in case we checked all streams with some keywords and got for example 50 streams selected, but after that, we typed more symbols to search string and got only 10 results? In this case, those 50 streams will be still selected inside useBulkEditService while only 10 will be visible to the user

Hmm... Interesting use-case. Probably we need to unselect all selected streams when the user changes filter settings.
@edmundito WDYT?

@dizel852
Copy link
Contributor

Found a bug while testing:

  1. Filter the streams and select "all"
  2. Bulk edit and hit apply
  3. Unselect the streams

You will notice that there is only the streams that were filtered, but the others are gone!

Confirm.

Copy link
Contributor

@dizel852 dizel852 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to fix the issue

Copy link
Contributor

@edmundito edmundito left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally and works as expected. Tested:

  • Editing streams before and after bulk edit
  • Cancel and Save
  • Editing filtered streams with bulk edit

@edmundito
Copy link
Contributor

I just read the question about changing the search during bulk editing. I think the best option is to disable the search input while bulk edit is enabled.

Copy link
Contributor

@edmundito edmundito left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested with disabled search, LGTM

@edmundito edmundito requested a review from dizel852 February 6, 2023 15:32
Copy link
Contributor

@dizel852 dizel852 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/frontend Related to the Airbyte webapp
Projects
None yet
Development

Successfully merging this pull request may close these issues.

All streams are selected for bulk edit after they are filtered by keyword
4 participants