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

fix(editor): Fix execution list item selection #5606

Merged
merged 28 commits into from
Mar 17, 2023

Conversation

cstuncsik
Copy link
Contributor

No description provided.

@n8n-assistant n8n-assistant bot added n8n team Authored by the n8n team ui Enhancement in /editor-ui or /design-system labels Mar 3, 2023
@@ -229,7 +224,7 @@
</div>
<div v-else :class="$style.loadedAll">{{ $locale.baseText('executionsList.loadedAll') }}</div>
</div>
<div v-if="checkAll === true || isIndeterminate === true" :class="$style.selectionOptions">
<div v-if="numSelected > 0" :class="$style.selectionOptions">
<span>
{{ $locale.baseText('executionsList.selected', { interpolate: { numSelected } }) }}
Copy link
Contributor

Choose a reason for hiding this comment

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

does not handle plural case. should be 1 execution selected or 10 executions selected
Bildschirmfoto 2023-03-06 um 09 50 23

Copy link
Member

Choose a reason for hiding this comment

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

Done by Csaba

@change="handleCheckAllChange"
label=""
/>
<el-checkbox :value="checkAll" @change="handleCheckAllChange" label="" />
Copy link
Contributor

Choose a reason for hiding this comment

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

this seems like an intentional feature we are losing here.. can you make sure to double check with Product and design first?

@cstuncsik cstuncsik requested a review from mutdmour March 7, 2023 16:10
@@ -36,14 +36,26 @@
</el-checkbox>
</div>

<el-checkbox
Copy link
Contributor

Choose a reason for hiding this comment

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

can you please add tests for this new feature

@@ -285,7 +301,8 @@ export default mixins(externalHooks, genericHelpers, executionHelpers, restApi,
finishedExecutionsCount: 0,
finishedExecutionsCountEstimated: false,

checkAll: false,
allVisibleSelected: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

this is the problem that I shared.. we should not have a flag for this.. it should be a set of ids instead..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added it because when all existing is selected there is a filter prop (deleteBefore) created to tell the server which should be deleted.
In this case there are no IDs added to theselectedItems (however we could do that in some cases that could be too much)

@github-actions
Copy link
Contributor

github-actions bot commented Mar 9, 2023

Great PR! Please pay attention to the following items before merging:

Files matching packages/**:

  • If fixing bug, added test to cover scenario.
  • If addressing forum or Github issue, added link to description.

Files matching packages/**/*.ts:

  • Added unit tests to cover new or updated functionality.

Files matching **/*.vue:

  • Used composition API for all new components.
  • Added component or unit tests to cover functionality.

Files matching packages/editor-ui/**/*.vue:

  • Added E2E if adding new features.
  • Used design system tokens (colors, spacings...) where possible.

Make sure to check off this list before asking for review.

@codecov
Copy link

codecov bot commented Mar 10, 2023

Codecov Report

Patch coverage has no change and project coverage change: +0.09 🎉

Comparison is base (541850f) 13.05% compared to head (6bedc7f) 13.15%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5606      +/-   ##
==========================================
+ Coverage   13.05%   13.15%   +0.09%     
==========================================
  Files        2462     2462              
  Lines      112686   112690       +4     
  Branches    17498    17516      +18     
==========================================
+ Hits        14715    14825     +110     
+ Misses      97471    97366     -105     
+ Partials      500      499       -1     
Impacted Files Coverage Δ
packages/cli/src/executions/executions.service.ts 9.00% <0.00%> (-0.18%) ⬇️

... and 9 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@cstuncsik cstuncsik merged commit 7a352ef into master Mar 17, 2023
@cstuncsik cstuncsik deleted the pay-101-unselect-a-single-execution-in-execution branch March 17, 2023 05:18
@n8n-assistant n8n-assistant bot added the Upcoming Release Will be part of the upcoming release label Mar 17, 2023
@janober
Copy link
Member

janober commented Mar 26, 2023

Got released with [email protected]

@janober
Copy link
Member

janober commented Mar 26, 2023

Got released with [email protected]

sunilrr pushed a commit to fl-g6/qp-n8n that referenced this pull request Apr 24, 2023
* fix(editor): Fix execution list item selection

* fix(editor): Delete only selected executions

* fix(editor): Fix clear selection

* fix(editor): Fix clear selection

* fix(editor): Fix clear selection

* feat(editor): Add select all existing executions checkbox

* fix(editor): Do not mark later loaded executions selected

* test(editor): Add execution list unit test

* fix(editor): Fix selection

* test(editor): update execution selection test

* fix(editor): Handle UI state when there is no execution

* fix(editor): Remove unnecessary logic

* test(editor): Add more execution list unit tests and fake data generation

* test(editor): Add more execution list unit tests

* test(editor): Simplifying test setup

* chore: update pnpm lock after resolving merge conflocts

* chore: fix package version

* fix: Improved executions deletion to prevent crashing and fixed removal of failed executions

* fix: Add comment to clarify why change was needed

* fix: fix executions list bug when selecting all and changing filter

* fix: fix execution lists running execution showing up on different workflow id

* fix(editor): Deleting an execution while all are selected

* fix(editor): Deleting an execution while all are selected

---------

Co-authored-by: Omar Ajoue <[email protected]>
Co-authored-by: Alex Grozav <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
n8n team Authored by the n8n team Released ui Enhancement in /editor-ui or /design-system Upcoming Release Will be part of the upcoming release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants