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.

Sign up for GitHub

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

Jump to bottom

refactor(core): Update tag endpoints to use DTOs and injectable config #12380

Merged
merged 22 commits into from
Jan 9, 2025
Merged

refactor(core): Update tag endpoints to use DTOs and injectable config #12380

merged 22 commits into from
Jan 9, 2025

Conversation

RicardoE105
Copy link
Contributor

@RicardoE105 RicardoE105 commented Dec 26, 2024 •
edited
Loading

Summary

Title self explanatory

I have no clue why we have the N8N_WORKFLOW_TAGS_DISABLED env variable at all. Not sure in which scenario we want to disable tags 🤔. Removing this env variable will simplify things as we need to constantly check this value. @netroy any idea why?

Review / Merge checklist

  • PR title and summary are descriptive. (conventions)
  • Docs updated or follow-up ticket created.
  • Tests included.
  • PR Labeled with release/backport (if the PR is an urgent fix that needs to be backported)

@n8n-assistant n8n-assistant bot added core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team ui Enhancement in /editor-ui or /design-system labels Dec 26, 2024
@codecov Codecov
Copy link

codecov bot commented Dec 26, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 67.27273% with 18 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
packages/editor-ui/src/api/tags.ts 30.00% 7 Missing ⚠️
packages/editor-ui/src/stores/tags.store.ts 0.00% 3 Missing ⚠️
packages/@n8n/api-types/src/dto/index.ts 0.00% 2 Missing ⚠️
packages/cli/src/server.ts 0.00% 2 Missing ⚠️
packages/cli/src/workflows/workflow.service.ts 60.00% 0 Missing and 2 partials ⚠️
packages/@n8n/config/src/index.ts 50.00% 0 Missing and 1 partial ⚠️
...ckages/cli/src/workflow-execute-additional-data.ts 0.00% 0 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

@RicardoE105 RicardoE105 requested a review from netroy December 26, 2024 22:09
@RicardoE105 RicardoE105 changed the title refactor(core): Update tag endpoints to use DTOs refactor(core): Update tag endpoints to use DTOs and injectable config Dec 26, 2024
@RicardoE105 RicardoE105 requested review from netroy and removed request for netroy December 26, 2024 22:39
netroy
netroy requested changes Dec 27, 2024
View reviewed changes
Copy link
Member

@netroy netroy left a comment

Choose a reason for hiding this comment

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

couple of minor comments, but looks good so far. 🙇🏽

packages/cli/src/controllers/tags.controller.ts Outdated Show resolved Hide resolved
packages/@n8n/config/src/configs/tags.config.ts Outdated Show resolved Hide resolved
packages/@n8n/api-types/src/dto/tag/base-tag-request.dto.ts Outdated Show resolved Hide resolved
packages/cli/src/controllers/tags.controller.ts Outdated Show resolved Hide resolved
packages/cli/src/controllers/tags.controller.ts Show resolved Hide resolved
packages/cli/src/databases/repositories/workflow.repository.ts Outdated Show resolved Hide resolved
packages/cli/src/databases/repositories/workflow.repository.ts Outdated Show resolved Hide resolved
packages/cli/src/workflows/workflow.service.ts Outdated Show resolved Hide resolved
packages/cli/test/integration/security-audit/instance-risk-reporter.test.ts Outdated Show resolved Hide resolved
packages/@n8n/api-types/src/dto/tag/retrieve-tag-query.dto.ts Outdated Show resolved Hide resolved
@cypress Cypress
Copy link

cypress bot commented Jan 9, 2025 •
edited
Loading

n8n    Run #8700

Run Properties:  status check passed Passed #8700  •  git commit b1a40a231b: 🌳 master 🖥️ browsers:node18.12.0-chrome107 🤖 PR User 🗃️ e2e/*
Project n8n
Branch Review master
Run status status check passed Passed #8700
Run duration 04m 49s
Commit git commit b1a40a231b: 🌳 master 🖥️ browsers:node18.12.0-chrome107 🤖 PR User 🗃️ e2e/*
Committer Ricardo Espinoza
View all properties for this run ↗︎
Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 489
View all changes introduced in this branch ↗︎

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jan 9, 2025

✅ All Cypress E2E specs passed

@RicardoE105 RicardoE105 merged commit b1a40a2 into master Jan 9, 2025
41 of 42 checks passed
@RicardoE105 RicardoE105 deleted the add-dtos-to-tag-controller branch January 9, 2025 19:17
@Maticaba99 Maticaba99 mentioned this pull request Jan 9, 2025
Merged
4 tasks
riascho pushed a commit that referenced this pull request Jan 14, 2025
@RicardoE105 @riascho
refactor(core): Update tag endpoints to use DTOs and injectable config (
ba4b3bc 
#12380)
@janober
Copy link
Member

janober commented Jan 15, 2025

Got released with n8n@1.75.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@netroy netroy netroy approved these changes

Assignees
No one assigned
Labels
core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team Released ui Enhancement in /editor-ui or /design-system
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@RicardoE105 @janober @netroy