-
Notifications
You must be signed in to change notification settings - Fork 650
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
Remove Data Observatory v1 #16207
Remove Data Observatory v1 #16207
Conversation
This pull request has been linked to Clubhouse Story #111330: Remove DO v1 from Builder. |
...s/test/spec/builder/editor/layers/layer-content-view/analyses/analysis-controls-view.spec.js
Show resolved
Hide resolved
...lder/editor/layers/layer-content-view/analyses/analyses-quota/analyses-quota-options.spec.js
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for taking care of this @Jesus89, this will make much easier the maintenance of the old codebase 🎉
I have very little knowledge about how the old DO itself works, but from outside I've spotted a couple of details.
Apart from the linter comment, I also miss a database migration which drops the old Data Observatory columns. It should be something similar to this.
Thanks for the review. Regarding the columns drop, I would make this in a different step (in the future). I've created a ticket with the code of the migration: https://app.clubhouse.io/cartoteam/story/145308/add-migration-to-drop-old-do-columns. |
…:CartoDB/cartodb into chore/ch111330/remove-do-v1-from-builder
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Jesus89 Everything looks reasonable for the FrontEnd part. 👍🏻
I've just had a look at application.rb and there are some entries that I think we should also remove from the precompile list: data_library_deps.js, data_library.js and data_library_deps.css
Thanks for the review @VictorVelarde. I'll clean these entries from the application config:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀 🌔
NOTE: this can not be merged/deployed until March 31st