User table refactor 2 #11531

javitonino · 2017-02-14T12:20:05Z

Risk meter: 🔴🔴🔴⚪️⚪️
Adding creation callbacks to Carto::UserTable, banning mass assignment from Carto::UserTable. But they should have no impact since we are not creating new tables using the AR model in production code. Moving boundaries code in map models.

In this PR I worked on the table creation process. Main changes:

Implemented canonical visualization creation (In app/model_factories/carto/*)
Call the service for physical table creation
Block attribute mass assignment in Carto::UserTable (ported from the Sequel model). This is needed to make sure the name is set through the Table service, which will make checks to ensure the name is valid and available. I don't like it, but it's quite difficult to change due to hidden dependencies and we have enough in our plate just with the ORM change.

Also I did a lot of changes to tests to facilitate development. I split the tests for the service in two sets, the ones which work with both models right now, and the ones which work with just the old user table model. To continue the refactor, you can enable the full suite for both models at the bottom of table_spec.rb and see what is red. This is the bulk of PR lines (moving a bunch of stuff around) and it is only for ease of testing. We can consider reverting this.

Missing: checking named maps updates and friends. They probably work due to some model hooks, and we can check later in the next steps (implement model update and deletion).

Suggested next steps: The main thing broking tests right now is table renaming. Mainly because the tests in table_spec can depend on the previous specs, so if renaming fails, it causes naming conflicts for future examples. 😞

Acceptance: similar to #11508

houndci-bot · 2017-02-14T12:20:19Z

app/model_factories/carto/overlay_factory.rb

+        template: '<div class="loader" original-title=""></div>',
+        options: { display: true, x: 20, y: 150 },
+      )
+    end


end at 33, 4 is not aligned with def at 26, 3.

houndci-bot · 2017-02-14T12:20:19Z

app/model_factories/carto/overlay_factory.rb

+    end
+
+   def build_loader_overlay(order)
+      Carto::Overlay.new(


Use 2 (not 3) spaces for indentation.

houndci-bot · 2017-02-14T12:20:19Z

app/model_factories/carto/overlay_factory.rb

+      )
+    end
+
+   def build_loader_overlay(order)


Inconsistent indentation detected.

houndci-bot · 2017-02-14T16:07:13Z

app/models/carto/map.rb

+    longitude_size = bounds[:maxx] - bounds[:minx]
+
+    # Don't touch zoom if the table is empty or has no bounds
+    return if longitude_size == 0 && latitude_size == 0


Use longitude_size.zero? instead of longitude_size == 0.
Use latitude_size.zero? instead of latitude_size == 0.

juanignaciosl · 2017-02-15T09:25:00Z

WIP: acceptance suggestions:

After the creation + ghost tables...

Create a map based on that table.
Rename the table though SQL API. Check that ghost tables rename the dataset and the map still works.
Drop the table through SQL API. Ghost tables should delete the dataset and the map.

juanignaciosl

I've done some comments, most of them minor. Looks great! I haven't reviewed table_spec yet, though. Italics are just notes for easier tracking.

juanignaciosl · 2017-02-15T11:59:21Z

app/model_factories/carto/layer_factory.rb

@@ -0,0 +1,73 @@
+module Carto
+  module LayerFactory
+    def build_default_base_layer(user)


This method is a direct translation of ModelFactories::LayerFactory#get_default_base_layer.

juanignaciosl · 2017-02-15T12:02:34Z

app/model_factories/carto/layer_factory.rb

+      Carto::Layer.new(options)
+    end
+
+    def build_default_labels_layer(base_layer)


This method is a direct translation of ModelFactories::LayerFactory#get_default_labels_layer.

juanignaciosl · 2017-02-15T12:03:36Z

app/model_factories/carto/layer_factory.rb

+      )
+    end
+
+    def build_data_layer(user_table)


This method is a translation of ModelFactories::LayerFactory#get_default_data_layer.

Nice refactor of method parameters 👌

juanignaciosl · 2017-02-15T12:11:49Z

app/model_factories/carto/layer_factory.rb

+
+    def build_data_layer(user_table)
+      user = user_table.user
+      geometry_type = user_table.service.the_geom_type || 'geometry'


Defining user_table.the_geom_type would avoid coupling to user_table.service. The convention of returning 'geometry' by default probably belongs there, too.

juanignaciosl · 2017-02-15T12:14:17Z

app/model_factories/carto/layer_factory.rb

@@ -0,0 +1,73 @@
+module Carto
+  module LayerFactory
+    def build_default_base_layer(user)


Minor: as these are factories, maybe you can omit build_ method prefix, since all factory methods should be meant for building.

I added build_ as opposed to create_, i.e: this doesn't persist the visualization. Also, since this is amixin mant to be included, I thought it best to have names less likely to cause collisions.

Ok, good point 👍 I'd like keeping the convention of never persisting implicitly, but we're not there yet.

juanignaciosl · 2017-02-15T13:07:40Z

app/models/table.rb

    else
      @user_table = args[:user_table]
    end
    # TODO: this probably makes sense only if user_table is not passed as argument
    @user_table.set_service(self)
  end

+  def model_class
+    ::UserTable


Anticipation of future replacement? Add a comment, in that case.

Testing purposes, really. It's so we can run service tests against both models (by stubbing this). I'll add a comment.

juanignaciosl · 2017-02-15T13:09:20Z

spec/factories/user_tables.rb

+      user_table.unstub(:create_canonical_visualization)
+    end
+
+    trait :with_db_table do


Maybe the default should be "with_db_table", as it's the most accurate behaviour, don't you think?

That really increases the testing time and breaks some tests. Long story short, this is called from create_full_visualization which works with Carto::User. And a lot of the newer tests create users without databases, so trying to create the physical table broke them.

So I decided to keep the current behaviour by default. My personal trade-off would be to add with_canonical_visualization by default, as that is more accurate than trying to create it in the test (and less repetitive). I don't like creating the table by default, as that forces to create user database as well, it's not needed in most cases, and it's slow.

Ok, let's follow that approach, makes sense 👍

juanignaciosl · 2017-02-15T13:11:12Z

app/models/carto/user_table.rb

    def self.column_defaults
      # AR sets privacy = 0 (private) by default, taken from the DB. We want it to be `nil`
      # so the `before_validation` hook sets an appropriate privacy based on the table owner
      super.merge("privacy" => nil)
    end

+    attr_accessible :privacy, :tags, :description


This is related to your comment about mass assignment blocking, isn't it? Maybe it should be a comment here.

Yes, I'll try to explain it here. It's tricky to explain exactly, but at least something to raise awareness of the issue can be nice.

juanignaciosl · 2017-02-15T13:11:28Z

spec/models/carto/user_table_spec.rb

@@ -10,7 +10,11 @@

    @user = FactoryGirl.create(:carto_user)
    @carto_user = @user
-    @user_table = Carto::UserTable.new(user: @user, name: unique_name('user_table'))
+
+    @user_table = Carto::UserTable.new


This is now forbidden because of attr_accessible, isn't it?

juanignaciosl · 2017-02-15T13:11:36Z

spec/models/carto/user_table_spec.rb

@@ -23,7 +27,9 @@

  it_behaves_like 'user table models' do
    def build_user_table(attrs = {})
-      Carto::UserTable.new(attrs)
+      ut = Carto::UserTable.new
+      ut.assign_attributes(attrs, without_protection: true)


This is also forbidden because of attr_accessible, isn't it?

Exactly, I had to use extreme measures to make it work. The named parameter is pretty nice for that.

javitonino · 2017-02-16T09:05:38Z

Ready for second round. I'd recommend checking the commit list to see what's changed (or even reviewing per-commit, I tried to make them pretty self-contained this time :D).

gfiorav

I salute you for the CARTO public service you have done ❤️

Really great extraction. Very satisfying to see bad practices bite the dust in pro of better extraction and orthogonalisation. Very good work.

I've left some comments I'd like addressed. Most are design comments. Let's debate!

gfiorav · 2017-02-16T11:40:56Z

app/model_factories/carto/overlay_factory.rb

+        build_zoom_overlay(6),
+        build_loader_overlay(8)
+      ]
+      overlays << build_logo_overlay(9) unless user.has_feature_flag?('disabled_cartodb_logo')


Maybe we should have used configuration for this!

gfiorav · 2017-02-16T11:41:51Z

app/model_factories/carto/overlay_factory.rb

+      Carto::Overlay.new(
+        order: order,
+        type: "logo",
+        template: '',


This could be a default in the model, so there's no need to pass an empty string in the constructor.

I'll take a look and see if there is any problem in this being a default, or we are relying on it being nil in some other place.

This can be set to null from the frontend (via controllers) and .carto restores. Don't know if it makes sense, but I do not feel confident changing behaviour for those.

I think that doing what I proposed is best in this case, it's your call!

But there are some parts of the code that want to save nil instead of empty string!

gfiorav · 2017-02-16T11:42:05Z

app/model_factories/carto/overlay_factory.rb

+      Carto::Overlay.new(
+        order: order,
+        type: 'search',
+        template: '',


gfiorav · 2017-02-16T11:44:02Z

app/model_factories/carto/visualization_factory.rb

+  class VisualizationFactory
+    def self.build_canonical_visualization(user_table)
+      kind = user_table.raster? ? Carto::Visualization::KIND_RASTER : Carto::Visualization::KIND_GEOM
+      esv = user_table.external_source_visualization


This whole PR is beautiful. I'd stay away from acronyms, it's the only thing making my eyes jump to the top of this file every so often to check what it meant. Don't break the graceful flow downwards 😢

gfiorav · 2017-02-16T11:47:23Z

app/models/carto/map.rb

@@ -3,7 +3,55 @@
 require_relative '../../helpers/bounding_box_helper'
 require_relative './carto_json_serializer'

+module Carto::MapBoundaries


Please, let's separate classes and modules to separate files. It's off-putting to come into a model and scroll through a whole module!

gfiorav · 2017-02-16T11:59:00Z

app/models/carto/user_table.rb

@@ -132,6 +156,10 @@ def permission
      visualization.permission if visualization
    end

+    def external_source_visualization
+      data_import.try(:external_data_imports).try(:first).try(:external_source).try(:visualization)


General comment totally ignorable:

I know I've contributed to it, but I have mixed feelings about the .try explosion.

It's good when you can't trust the data structure to be populated but let's make sure we design new data structures so that they don't require so much trying

gfiorav · 2017-02-16T11:59:57Z

app/models/carto/visualization.rb

+  def set_random_id
+    # This should be done with a DB default
+    self.id ||= random_uuid
+  end


This is an ActiveRecord! Can't we make the db do this now?

We could, but that requires a DB migration on the visualization table, and you know the current situation around that.

gfiorav · 2017-02-16T12:00:32Z

app/models/carto/visualization.rb

@@ -82,6 +84,11 @@ class Carto::Visualization < ActiveRecord::Base
  validates :version, presence: true

  before_validation :set_default_version
+  before_create :set_random_id, :set_default_permission
+
+  # INFO: workaround for array saves not working


Please elaborate, it's really weird! The comment needs more info :)

It's an old comment, I just moved it :D

Basically, there is a bug in activerecord-postgresql-array so it doesn't work on inserts, but it does work with updates. I'll expand a bit on the comment.

I'd make the comment better then, explaining the two methods and the delay part. It's worth the explanation IMHO.

gfiorav · 2017-02-16T12:02:26Z

app/models/table.rb

+  # This is here just for testing purposes (being able to test this service against both models)
+  def model_class
+    ::UserTable
+  end


I know the pain you went through with this, but I feel that code shouldn't be changed to make a test framework work.

How about adding a TODO: in the comment and packing some info on when this can be removed? (i.e.: remove when on rspec x.y.z)

It doesn't have to do with rspec at all. It's just so I can stub the class used by the service, so I can run the same specs against the two models.

The thing is, most tests related to user table are used through this service, so this allows me to select which model to user (which is used in the initializers).

This will be removed when we delete ::UserTable.

Umm I'd favor some .class.parents magic in tests instead then. I still maintain that code shouldn't change to fit tests better.

Let's leverage anyway. @juanignaciosl what's your take?

How could you? I mean, the problem is that when the test does ::Table.new, this instantiates a ::UserTable. I'd like for it to be a Carto::UserTable so this service calls the new model for metadata persistence. I really could not find any way to do this without changing production code 😢

Isn't there something to be done with .ancestors o something like .superclass (although no inheritance here)?

That could help in order to get the model and do something different in the test depending on it. But the problem is that we want this model (see initialize above) to instantiate different model classes, so we have to tell this service, which model class to instantiate.

gfiorav · 2017-02-16T12:03:09Z

spec/factories/user_tables.rb

+      user_table.unstub(:create_canonical_visualization)
+    end
+
+    trait :with_db_table do


Nice ❤️

javitonino · 2017-02-16T13:28:23Z

I fixed some things. Pending discussion:

Avoid touching ::Table service production code to allow testing for different models?
Migration to change user_tables.privacy default instead of hacking the model?
Moving modules shared between models to a new file?

javitonino · 2017-02-16T14:12:24Z

Tests are 🍏. Just waiting for discussion on the topics. IMHO:

No other way around touching ::Table code for testing
Agree with @juanignaciosl. Not the right time to do migrations in the middle of the refactor
This is just an excuse to finish the refactor quickly so we can remove the old module and merge the models 😛

Anyway, 👍 or 👎

juanignaciosl · 2017-02-16T14:16:30Z

@gfiorav 's call, isn't it? Let me know if I should do anything else.

gfiorav · 2017-02-16T14:43:28Z

At the very least, I'd move the module to its file. I've learned about Carto::Layer and think about it evey time I open it!

javitonino · 2017-02-16T15:09:24Z

Extracted map common module. I started a new directory (models/common) for this purpose.

gfiorav

Made a comment!

gfiorav · 2017-02-16T15:30:42Z

app/models/common/map_common.rb

@@ -0,0 +1,45 @@
+module Carto::MapBoundaries


I don't like this folder here too much :D

It isn't a model, and it's part of our carto code, so I'd say lib/carto/map_boundaries.rb

I disagree in both counts 😆

It's not a model, but it's an obligatory part of a model. This module cannot be used anywhere else than the model (it uses model methods), and the model depends on this module (the model calls methods on this module).

Also, we use carto for things of the newer models. But this is not the case, this is common for both the old and new models. But well, that's the lesser part.

I really think we should keep this somewhere close to the models as it is so tightly related with them.

Also note. We should consider deleting this module and copying the code it contains to the model when we finish the refactor, because at that point, it will be a module used in only one place.

Hehe, I guess there is the discrepancy: I think it's nice to have this out of the model in any case. Service like. Do what you think is best 👍

That's why I mentioned it, I thought it might be the case. There is already kind of a service in here: BoundingBoxHelper which does part of the job. We could refactor some of this code into that service, but will still need a part of the code to copy the result from that service to the map model.

Anyway, I don't like touching the map code in the user table refactor anyway, so I prefer to leave it as is, and maybe get to it later. We have a limited time to refactor, and ::UserTable is the one that being bad to us.

juanignaciosl · 2017-02-17T12:49:01Z

As a reminder that this is not the cause, I've detected #11571 during this acceptance.

juanignaciosl · 2017-02-17T13:01:25Z

"Do not merge" because this is not a Friday deploy, but it's ready 👍

javitonino · 2017-02-17T13:03:39Z

\o/. Modified issue title for extra visibility, you can never be too careful.

juanignaciosl · 2017-02-17T13:08:16Z

Well, I'm still assigned ;-)

javitonino · 2017-02-17T13:12:04Z

Sure! That's another great way to ensure it 💖

Javier Torres added 17 commits February 13, 2017 17:02

Merge branch 'master' into user_table_refactor_2

eb21620

Temporal testing stuff

29dec64

Protect attributes from mass assignment like in Sequel model

9153d94

First steps of carto visualization factory

1a12f82

More work on canonical map factory

c7a646c

Create canonical map on user table creation

080a38b

Add missing methods

6899859

Fix factories

f757b6e

More visualization factory

339c2a2

Visualization builder working

cecc5d2

Prepare table service to be tested with both models

d43c189

Starting to map test compatibility with different models

f5b63c7

Split table specs with models

55eef55

Serialize map bounds to JSON in new model

5b236b6

Make a test model independent

c051aa8

Fix Carto::UserTable model tests due to mass assigment protection

e8ecfdf

Merge branch 'master' into user_table_refactor_2

92c1b59

houndci-bot reviewed Feb 14, 2017

View reviewed changes

Javier Torres added 6 commits February 14, 2017 13:22

Hound

a6ab46c

Rollback map serializers changes

651e78a

Fix tests

290764a

Revert dataset controller change, and instead work in the factory

ce02eac

Add trait for full user table

25c1a09

Add default boundaries calculation

2e4760f

houndci-bot reviewed Feb 14, 2017

View reviewed changes

Javier Torres added 3 commits February 14, 2017 17:12

Hound

6d70277

Move map boundaries calculations to shared mixin

ee230d7

Forgot to move MAX_ZOOM

a97df3f

juanignaciosl suggested changes Feb 15, 2017

View reviewed changes

javitonino added 7 commits February 16, 2017 09:18

Move tags saving workaround to visualization model

fd3f31f

Move default permission creation to visualization model

f5f85b5

Add coment about mass protection assignment

f2ad637

Comment about ::Table.model_class

ed72822

Add test for map center default value

ecc1113

Move map default options to initialization hook

e0e7b95

Add documentation on map fields serialization

bd8489a

gfiorav suggested changes Feb 16, 2017

View reviewed changes

javitonino added 3 commits February 16, 2017 14:20

Move comment about serialized fields in map

6041111

Change new to new_record for more future preparedness

7c66e34

Expand comment on tags hack

a59caad

Extract map model shared module to own file

932984c

gfiorav suggested changes Feb 16, 2017

View reviewed changes

gfiorav approved these changes Feb 16, 2017

View reviewed changes

juanignaciosl added the do not merge yet label Feb 17, 2017

juanignaciosl approved these changes Feb 17, 2017

View reviewed changes

juanignaciosl removed the do not merge yet label Feb 20, 2017

juanignaciosl merged commit 38d91b7 into master Feb 20, 2017

juanignaciosl deleted the user_table_refactor_2 branch February 20, 2017 08:08

User table refactor 2 #11531

User table refactor 2 #11531

Conversation

javitonino commented Feb 14, 2017 • edited by juanignaciosl Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juanignaciosl commented Feb 15, 2017

juanignaciosl left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javitonino commented Feb 16, 2017

gfiorav left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javitonino commented Feb 16, 2017

javitonino commented Feb 16, 2017

juanignaciosl commented Feb 16, 2017

gfiorav commented Feb 16, 2017

javitonino commented Feb 16, 2017

gfiorav left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juanignaciosl commented Feb 17, 2017

juanignaciosl commented Feb 17, 2017

javitonino commented Feb 17, 2017

juanignaciosl commented Feb 17, 2017

javitonino commented Feb 17, 2017

javitonino commented Feb 14, 2017 •

edited by juanignaciosl

Loading

juanignaciosl left a comment •

edited

Loading