feat(core): Add migration to add property userActivated to user settings (no-changelog) #5940

RicardoE105 · 2023-04-10T23:05:10Z

Context: ADO-460

github-actions · 2023-04-10T23:05:41Z

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 packages/cli/src/databases/migrations/**:

Requested review from at least two engineers on migration.
Avoided irreversible data migrations.
Avoided deleting or updating data keys.
Wrote 'down' migration if possible.

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

krynble

Migration is failing for MySQL but I'm not sure why. No errors are reported.

RicardoE105 · 2023-04-11T16:07:20Z

@krynble quite weird as I tested it. Will have a look. THanks

RicardoE105 · 2023-04-11T16:16:36Z

@krynble cannot replicate the issue. It works just fine for me. Are you starting n8n with a new database?

netroy · 2023-04-11T16:45:31Z

tests failing on the CI as well https://github.com/n8n-io/n8n/actions/runs/4669890935/jobs/8268940696

packages/cli/src/databases/migrations/mysqldb/1681134145996-AddUserActivatedProperty.ts

codecov · 2023-04-12T17:39:18Z

Codecov Report

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

Comparison is base (cd89489) 18.49% compared to head (2727b35) 18.64%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5940      +/-   ##
==========================================
+ Coverage   18.49%   18.64%   +0.14%     
==========================================
  Files        2563     2583      +20     
  Lines      116018   116471     +453     
  Branches    18118    18179      +61     
==========================================
+ Hits        21458    21714     +256     
- Misses      93930    94119     +189     
- Partials      630      638       +8

see 84 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.

packages/cli/src/databases/migrations/mysqldb/1681134145996-AddUserActivatedProperty.ts

github-actions · 2023-04-14T10:02:14Z

✅ All Cypress E2E specs passed

RicardoE105 · 2023-04-18T17:14:52Z

@krynble ended up stringifying the setting and it works with both scenarios (when the query returns a string and when it returns an object) and it's more simple than having an IF to do one or the other.

krynble

LGTM, working for all databases and set up.

github-actions · 2023-04-19T14:26:22Z

✅ All Cypress E2E specs passed

ivov

Tested on sqlite, MySQL, Postgres.

Commented on one migration but also applies to the others.

packages/cli/src/Interfaces.ts

packages/cli/src/databases/migrations/mysqldb/1681134145996-AddUserActivatedProperty.ts

packages/cli/src/databases/migrations/postgresdb/1681134145996-AddUserActivatedProperty.ts

github-actions · 2023-04-20T13:56:01Z

✅ All Cypress E2E specs passed

github-actions · 2023-04-20T14:47:08Z

✅ All Cypress E2E specs passed

github-actions · 2023-04-20T14:48:48Z

✅ All Cypress E2E specs passed

github-actions · 2023-04-20T17:12:19Z

✅ All Cypress E2E specs passed

github-actions · 2023-04-20T17:28:00Z

✅ All Cypress E2E specs passed

ivov

Thanks for addressing!

One doubt I still have:

Re: this comment, if the value at user.settings before the up migration was NULL, running a down migration with JSON_REMOVE() will not restore it back to NULL, i.e., it will only remove the new property and if it was the only one, we will end up with {}. Same for Postgres using -. But let me know if I'm misunderstanding.

github-actions · 2023-04-21T07:45:50Z

✅ All Cypress E2E specs passed

RicardoE105 · 2023-04-21T14:24:31Z

Re: this comment, if the value at user.settings before the up migration was NULL, running a down migration with JSON_REMOVE() will not restore it back to NULL, i.e., it will only remove the new property and if it was the only one, we will end up with {}. Same for Postgres using -. But let me know if I'm misunderstanding.

I see now what you mean. Previously null values will be now {}. I guess I can update all {} to be null after the first query runs.

github-actions · 2023-04-21T15:07:36Z

✅ All Cypress E2E specs passed

* master: feat(core): Add migration to add property userActivated to user settings (no-changelog) (#5940) feat(core): Add license:info command (#6047) feat: Replace this.$refs.refName as Vue with InstanceType<T> (no-changelog) (#6050) refactor(editor): Turn titleChange mixin to composable (#6059) test: Add stickies tests (#5413) refactor: Patch to adjust `consistent-type-imports` (no-changelog) (#6057) # Conflicts: # packages/editor-ui/src/components/ExecutionsView/ExecutionPreview.vue # packages/editor-ui/src/mixins/workflowRun.ts

* master: (47 commits) feat: Replace Vue.extend with defineComponent in editor-ui (no-changelog) (#6033) feat(core): Add migration to add property userActivated to user settings (no-changelog) (#5940) feat(core): Add license:info command (#6047) feat: Replace this.$refs.refName as Vue with InstanceType<T> (no-changelog) (#6050) refactor(editor): Turn titleChange mixin to composable (#6059) test: Add stickies tests (#5413) refactor: Patch to adjust `consistent-type-imports` (no-changelog) (#6057) fix(editor): Resolve expressions for grandparent nodes (#5859) ci(editor): Do not run parallel jobs for a single spec (no-changelog) (#6052) refactor(editor): Consolidate IN8nUISettings interface (#6055) refactor(core): Forbid raw enums (no-changelog) refactor(core): Sort variables files under variables folder (#6051) fix(core): Add breaking change record for domain and url matching (no-changelog) (#6048) feat(editor): Version control paywall (WIP) (#6030) feat(editor): Add disable template experiment (#5963) feat(core): Upgrade google-timezones-json to use the correct timezone for Sao Paulo (#6042) fix(Code Node): Update vm2 to address CVE-2023-30547 (#6039) docs: Add proprietary license text (no-changelog) (#6038) test(n8n Node): Unit tests (no-changelog) refactor: Accumulate `loadOptions` from all node versions to validate (no-changelog) (#6014) ... # Conflicts: # packages/cli/src/Server.ts

…ngs (no-changelog) (n8n-io#5940) * Add userActivated migration * Fix migration logic * Remove duplication when retrieving the activated users * Fix bug updating settings in mysql * Make userSettings type conform with naming convention * Disable naming convention rule only in IDatabaseCollections interface * Fix down method in Postgres migration * Reset '{}' to NULL when reversing migration

janober · 2023-05-02T10:16:40Z

Got released with [email protected]

Add userActivated migration

6f25d57

RicardoE105 requested a review from a team as a code owner April 10, 2023 23:05

RicardoE105 requested a review from krynble April 10, 2023 23:08

n8n-assistant bot added core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team labels Apr 10, 2023

krynble suggested changes Apr 11, 2023

View reviewed changes

RicardoE105 requested a review from krynble April 11, 2023 16:15

netroy reviewed Apr 11, 2023

View reviewed changes

packages/cli/src/databases/migrations/mysqldb/1681134145996-AddUserActivatedProperty.ts Outdated Show resolved Hide resolved

Fix migration logic

86bae30

RicardoE105 requested a review from netroy April 12, 2023 17:23

krynble suggested changes Apr 14, 2023

View reviewed changes

packages/cli/src/databases/migrations/mysqldb/1681134145996-AddUserActivatedProperty.ts Outdated Show resolved Hide resolved

RicardoE105 added 3 commits April 18, 2023 10:45

Sync master

610222f

Remove duplication when retrieving the activated users

8118cf7

Fix bug updating settings in mysql

17dc5e8

RicardoE105 requested a review from krynble April 18, 2023 17:13

krynble previously approved these changes Apr 19, 2023

View reviewed changes

ivov requested changes Apr 20, 2023

View reviewed changes

Make userSettings type conform with naming convention

cdc593c

RicardoE105 added 2 commits April 20, 2023 13:26

Disable naming convention rule only in IDatabaseCollections interface

f95c774

Fix down method in Postgres migration

031f0de

RicardoE105 dismissed krynble’s stale review via 031f0de April 20, 2023 17:27

RicardoE105 requested a review from ivov April 20, 2023 17:27

ivov reviewed Apr 21, 2023

View reviewed changes

Reset '{}' to NULL when reversing migration

2727b35

ivov approved these changes Apr 21, 2023

View reviewed changes

RicardoE105 merged commit 8a38624 into master Apr 21, 2023

RicardoE105 deleted the ado-460-add-useractivated-flag branch April 21, 2023 15:15

janober added the Released label May 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(core): Add migration to add property userActivated to user settings (no-changelog) #5940

feat(core): Add migration to add property userActivated to user settings (no-changelog) #5940

RicardoE105 commented Apr 10, 2023 •

edited

Loading

github-actions bot commented Apr 10, 2023

krynble left a comment

RicardoE105 commented Apr 11, 2023

RicardoE105 commented Apr 11, 2023

netroy commented Apr 11, 2023

codecov bot commented Apr 12, 2023 •

edited

Loading

github-actions bot commented Apr 14, 2023

RicardoE105 commented Apr 18, 2023

krynble left a comment

github-actions bot commented Apr 19, 2023

ivov left a comment •

edited

Loading

github-actions bot commented Apr 20, 2023

github-actions bot commented Apr 20, 2023

github-actions bot commented Apr 20, 2023

github-actions bot commented Apr 20, 2023

github-actions bot commented Apr 20, 2023

ivov left a comment •

edited

Loading

github-actions bot commented Apr 21, 2023

RicardoE105 commented Apr 21, 2023

github-actions bot commented Apr 21, 2023

janober commented May 2, 2023

feat(core): Add migration to add property userActivated to user settings (no-changelog) #5940

feat(core): Add migration to add property userActivated to user settings (no-changelog) #5940

Conversation

RicardoE105 commented Apr 10, 2023 • edited Loading

github-actions bot commented Apr 10, 2023

krynble left a comment

Choose a reason for hiding this comment

RicardoE105 commented Apr 11, 2023

RicardoE105 commented Apr 11, 2023

netroy commented Apr 11, 2023

codecov bot commented Apr 12, 2023 • edited Loading

Codecov Report

github-actions bot commented Apr 14, 2023

RicardoE105 commented Apr 18, 2023

krynble left a comment

Choose a reason for hiding this comment

github-actions bot commented Apr 19, 2023

ivov left a comment • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Apr 20, 2023

github-actions bot commented Apr 20, 2023

github-actions bot commented Apr 20, 2023

github-actions bot commented Apr 20, 2023

github-actions bot commented Apr 20, 2023

ivov left a comment • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Apr 21, 2023

RicardoE105 commented Apr 21, 2023

github-actions bot commented Apr 21, 2023

janober commented May 2, 2023

RicardoE105 commented Apr 10, 2023 •

edited

Loading

codecov bot commented Apr 12, 2023 •

edited

Loading

ivov left a comment •

edited

Loading

ivov left a comment •

edited

Loading