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

Adds a migration to remove _id from filter items #3490

Merged
merged 4 commits into from
Mar 8, 2021

Conversation

grafitto
Copy link
Contributor

@grafitto grafitto commented Mar 2, 2021

fixes #3485

PR checklist:

  • Update READ.me ?
  • Update API documentation ?

QA checklist:

  • Smoke test the functionality described in the issue
  • Test for side effects
  • UI responsiveness
  • Cross browser testing
  • Code review

@grafitto grafitto marked this pull request as ready for review March 3, 2021 13:36
describe('migration remove-_id-from-filter-items', () => {
beforeEach(async () => {
spyOn(process.stdout, 'write');
await testingDB.clearAllAndLoad(fixtures);
Copy link
Member

Choose a reason for hiding this comment

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

The migration looks good, and I believe it works as expected.

Still, I am concerned that the fixtures only show a single scenario: "A collection whose settings have filters, and those filters have nested items with _id present".

There are many other scenarios that need to be tested that they are not affected:

  • Collections that don't have filters
  • Collections that have filters and don't have nested items
  • Collections that have filters with nested items and no _id present in there.

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 have added a few test cases as suggested, you can check them out when you get time

@RafaPolit
Copy link
Member

While this migration may be needed, the fact that we have no clue as how to those _id's appeared there may leave us having to re-run an identical migration if this condition happens again. Please, spend some time trying to learn how this condition appeared in the first place, in order to first fix the problem, and only then run the migration.

@grafitto
Copy link
Contributor Author

grafitto commented Mar 5, 2021

While this migration may be needed, the fact that we have no clue as how to those _id's appeared there may leave us having to re-run an identical migration if this condition happens again. Please, spend some time trying to learn how this condition appeared in the first place, in order to first fix the problem, and only then run the migration.

I have tried creating new filters and grouped filters, both do not add _id into the database, There is a high chance we are dealing with old data, maybe before the decision was made to remove the field from the database. However I am not sure whether there are other cases where filters are creating other than the settings > filters section. If so you can let me know so that I check it out.

@konzz konzz merged commit 95fe25b into development Mar 8, 2021
@konzz konzz deleted the 3485-filters-validation-error-migration branch March 8, 2021 08:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Settings/Filters update throw a validation error when the instance has grouped filters
3 participants