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

Visualization backup service #14764

Merged
merged 20 commits into from
Mar 26, 2019
Merged

Visualization backup service #14764

merged 20 commits into from
Mar 26, 2019

Conversation

simon-contreras-deel
Copy link
Contributor

@simon-contreras-deel simon-contreras-deel commented Mar 21, 2019

Objectives:

  • Create visualization backup service
  • Create method
  • Use create method in visualization model
  • Restore method
  • Rake using restore method: bundle exec rake cartodb:vizs:restore_visualization['XXXXX']

Copy link
Contributor

@gonzaloriestra gonzaloriestra left a comment

Choose a reason for hiding this comment

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

Love the refactor! 😍

You forgot to add the new test file in Makefile, and I added some suggestions also.

app/services/carto/visualization_backup_service.rb Outdated Show resolved Hide resolved
app/services/carto/visualization_backup_service.rb Outdated Show resolved Hide resolved
app/services/carto/visualization_backup_service.rb Outdated Show resolved Hide resolved
lib/tasks/viz_maintenance.rake Show resolved Hide resolved
spec/services/carto/visualization_backup_service_spec.rb Outdated Show resolved Hide resolved
spec/services/carto/visualization_backup_service_spec.rb Outdated Show resolved Hide resolved
@gonzaloriestra
Copy link
Contributor

You still have to add the new test in Makefile 🙂

And one more thing I forgot to mention: instead of defining the service as a module, I'd do it as a regular class, as we do here for example: https://github.com/CartoDB/cartodb/blob/master/app/services/carto/overviews_service.rb

Modules are useful to provide some extra functionality to other classes, but in this case, I think a class fits better.

Copy link
Contributor

@javitonino javitonino 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 small comments. Nice extraction to a service!

app/services/carto/visualization_backup_service.rb Outdated Show resolved Hide resolved
app/services/carto/visualization_backup_service.rb Outdated Show resolved Hide resolved
@simon-contreras-deel
Copy link
Contributor Author

simon-contreras-deel commented Mar 22, 2019

You still have to add the new test in Makefile

Done!

About converting the service into a class, it could be a class with 2 static methods but I think it would add more bureaucracy and really, the service doesn't need to be an instance (it is even better), it doesn't have properties in common, ...

Said that as you know I have an important lack of RoR and for example, in VisualizationsExportService2 we are creating a class with only 2 modules, but I don't really understand why.

@gonzaloriestra
Copy link
Contributor

I agree that here we don't need to create an instance and the methods could be static, so the class is not so useful, but I don't know why there will be more bureaucracy. In fact, we could remove some includes... Anyway, it's just an opinion, both ways are valid.

Regarding VisualizationsExportService2, I guess it's a class to avoid having to include the modules. To be able to just do VisualizationsExportService2.new.import or VisualizationsExportService2.new.export.

@simon-contreras-deel
Copy link
Contributor Author

retest this please

@simon-contreras-deel simon-contreras-deel merged commit e0f947e into master Mar 26, 2019
@simon-contreras-deel simon-contreras-deel deleted the vis-backup-import branch March 26, 2019 08:45
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.

4 participants