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: Show actual execution data for production executions even if pin data exists #6302

Merged
merged 6 commits into from
Jun 1, 2023

Conversation

alexgrozav
Copy link
Contributor

No description provided.

@github-actions
Copy link
Contributor

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.

Files matching packages/editor-ui/src/mixins/**:

  • Avoided adding new mixins (use composables instead). Only removed code from here.

Files matching packages/editor-ui/src/views/NodeView.vue:

  • Avoided adding code here. Only refactored to make it smaller.

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

@codecov
Copy link

codecov bot commented May 23, 2023

Codecov Report

Patch coverage: 14.28% and project coverage change: -0.01 ⚠️

Comparison is base (2d13b3f) 27.65% compared to head (ef42bf9) 27.65%.

❗ Current head ef42bf9 differs from pull request most recent head 3c2e0ff. Consider uploading reports for the commit 3c2e0ff to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6302      +/-   ##
==========================================
- Coverage   27.65%   27.65%   -0.01%     
==========================================
  Files        2948     2948              
  Lines      180860   180927      +67     
  Branches    19642    19648       +6     
==========================================
+ Hits        50019    50033      +14     
- Misses     130094   130147      +53     
  Partials      747      747              
Impacted Files Coverage Δ
packages/editor-ui/src/views/NodeView.vue 13.48% <0.00%> (-0.02%) ⬇️
...ackages/nodes-base/nodes/RabbitMQ/RabbitMQ.node.ts 0.00% <0.00%> (ø)
.../nodes-base/nodes/RabbitMQ/RabbitMQTrigger.node.ts 0.00% <0.00%> (ø)
...kages/editor-ui/src/components/WorkflowPreview.vue 46.18% <16.66%> (-1.72%) ⬇️
packages/editor-ui/src/mixins/workflowHelpers.ts 14.49% <19.56%> (+0.65%) ⬆️

... and 22 files with indirect coverage changes

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

@n8n-assistant n8n-assistant bot added n8n team Authored by the n8n team ui Enhancement in /editor-ui or /design-system labels May 23, 2023
ivov
ivov previously approved these changes May 30, 2023
Copy link
Contributor

@ivov ivov left a comment

Choose a reason for hiding this comment

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

Confirmed working!

expect(result).toBe(true);
});

it('should return true if active execution is set and mode is manual', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
it('should return true if active execution is set and mode is manual', async () => {
it('should return true if active execution is set and mode is manual', () => {

expect(result).toBe(true);
});

it('should return false if active execution is set and mode is not manual', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
it('should return false if active execution is set and mode is not manual', async () => {
it('should return false if active execution is set and mode is not manual', () => {

});
}
if (shouldReplaceInputDataWithPinData()) {
const parentPinData = parentNode.reduce((acc: INodeExecutionData[], parentNodeName, index) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const parentPinData = parentNode.reduce((acc: INodeExecutionData[], parentNodeName, index) => {
const parentPinData = parentNode.reduce<INodeExecutionData[]>((acc, parentNodeName, index) => {

@@ -3604,6 +3604,8 @@ export default defineComponent({
type: 'error',
});
}
} else if (json && json.command === 'setActiveExecution') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
} else if (json && json.command === 'setActiveExecution') {
} else if (json?.command === 'setActiveExecution') {

Comment on lines 192 to 199
export function shouldReplaceInputDataWithPinData() {
const workflowsStore = useWorkflowsStore();
return (
!workflowsStore.activeWorkflowExecution ||
(workflowsStore.activeWorkflowExecution &&
workflowsStore.activeWorkflowExecution.mode === 'manual')
);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Or shorter:

export function shouldReplaceInputDataWithPinData() {
	const { activeWorkflowExecution } = useWorkflowsStore();
	return !activeWorkflowExecution || activeWorkflowExecution?.mode === 'manual';
}

Copy link
Contributor

Choose a reason for hiding this comment

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

We could make this a getter in the workflows store? To avoid adding to mixins.

@github-actions
Copy link
Contributor

✅ All Cypress E2E specs passed

@github-actions
Copy link
Contributor

github-actions bot commented Jun 1, 2023

✅ All Cypress E2E specs passed

@alexgrozav alexgrozav merged commit 4eb8437 into master Jun 1, 2023
23 checks passed
@alexgrozav alexgrozav deleted the pay-216-ndv-in-executions-view-shows-wrong-data branch June 1, 2023 16:12
MiloradFilipovic added a commit that referenced this pull request Jun 2, 2023
* master:
  fix(editor): Fix an issue with connections breaking during renaming (#6358)
  fix(core): Upgrade @n8n_io/riot-tmpl (no-changelog) (#6357)
  fix: Show actual execution data for production executions even if pin data exists (#6302)
  fix: Improve executions list polling performance (#6355)
  refactor: Revert 30k stars banner (no-changelog) (#6347)
  refactor: Add deprecation notice for basic auth and JWT auth (#6349)
  fix: Prevent redirect when outside of executions home (no-changelog) (#6305)
@janober
Copy link
Member

janober commented Jun 14, 2023

Got released with n8n@0.232.0

netroy pushed a commit that referenced this pull request Jun 14, 2023
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants