Cartodb: Torque layer dependencies not correctly registered

[javitonino]

Context

Due to a confusion with the meaning of Layer.data_layer and Map.data_layers (they are not the same), there are a bunch of pieces of code that are incorrect. This ticket illustrates one of them.

Steps to Reproduce

1. Create a map
2. Add a torque layer on dataset P
3. Add a join analysis to the torque layer, with dataset Q
4. Try to delete dataset Q

Current Result

You are not warned of a dependency with the map .

Expected result

You should be warned.

Additional info

The problem is that Carto::Map.update_dataset_dependencies calls data_layers which does not include torque layers. This mistake is repeated throughout the code (I'm thinking that the layer node style cache might also be affected).

My suggestion here would be to bite the bullet, and instead of fixing this specific issue (it's as easy as changing data_layers to carto_and_torque_layers), changing the behaviour of data_layers so it behaves consistently with Layer.data_layer?.

[javitonino]

This is the current status of the layer kind methods:

                  | Map models                   | Layer models
base              | all                          | tiled/bg/google/wms
data              | carto                        | carto/torque
user              | tiled/bg/google/wms          | tiled/bg/google/wms
carto_and_torque  | carto/torque                 | N/A
torque            | torque                       | torque
other             | torque                       | N/A
named_map         | carto/tiled/bg/google/wms    | carto/tiled/bg/google/wms
basemap           | N/A                          | tiled/google

My proposal is to standardize so:

base              | tiled/bg/google/wms
data              | carto/torque
user              | *Remove*
carto_and_torque  | *Remove*
torque            | torque
other             | *Remove*
named_map         | carto/tiled/bg/google/wms
basemap           | tiled/google

Alternatively, we could just fix the bug. Looking at how costly would be to change this.

[juanignaciosl]

@javitonino I'm beginning now the CR of this. Did you follow strictly that table? After reading it, removing the need of data mixing carto and torque is unexpected to me.

[javitonino]

Oh, that was a typo. I just edited the table. Indeed, data = carto + torque. I did not follow the entire table, I just removed carto_and_torque, and changed data to mean carto + torque.

I did not removed the other, nor user, and base_layers stills refers to everything.

I preferred the baby steps approach and keep myself to what fixes the current bug. We can open a refactor ticket if we want for the rest (I'm up for it!), although I'm not aware of any bug cause by that (but base_layers meaning everything lends itself to bugs).

[juanignaciosl]

Yep, I think that the proposal is not only great but also needed. Let's get the most out of your analysis work, open an issue with the steps (including this one).

Thanks for the explanation!

[juanignaciosl]

PS: visualization model is also involved here. I assume that your proposal includes changing visualization methods as well so they're the same as maps (they're mostly wrappers, now).

[javitonino]

For sure. I forgot about visualization model until I started implementing this. I created the follow up issue here: https://github.com/CartoDB/cartodb/issues/11553 and reflected the need to update the visualization models as well.