From 50e80281df053e5254d71c43f0dc8a326b837698 Mon Sep 17 00:00:00 2001 From: Nicklas Gummesson Date: Thu, 30 Apr 2015 11:26:15 +0200 Subject: [PATCH 1/8] Only add layer once confirmed it was added MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously even if the layer failed to be saved for whatever reason it was still added to the collection and thus appeared in the map’s collection. Using create and the wait: true prop it won’t add it until the server-side responded correctly. The server-side issue of layer being saved in inconsistent state still needs to be fixed in addition to this. --- lib/assets/javascripts/cartodb/models/map.js | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/lib/assets/javascripts/cartodb/models/map.js b/lib/assets/javascripts/cartodb/models/map.js index 47dfac0b1ae3..d08ababa6f50 100644 --- a/lib/assets/javascripts/cartodb/models/map.js +++ b/lib/assets/javascripts/cartodb/models/map.js @@ -504,7 +504,7 @@ cdb.admin.Map = cdb.geo.Map.extend({ return false; } - if (old) { + if (old) { // if the layer type is the same just update the values if (old.get('type') === layer.get('type')) { var old_attribution = old.get('attribution'); @@ -762,11 +762,9 @@ cdb.admin.Map = cdb.geo.Map.extend({ if (newLayer.wizard_properties.get('type') === 'torque' && self.layers.getTorqueLayers().length) { newLayer.wizard_properties.active('polygon'); } - self.layers.add(newLayer); - // check if there was error checking if the layer is added - if (newLayer.collection) { - newLayer.save(null, opts); - } + // wait: true is used to make sure the layer is not added until confirmed it was added successfully + // pass opts for success/error callbacks to be triggered as expected + self.layers.create(newLayer, _.extend({ wait: true }, opts)); } // Wait until the layer is totally ready in order to add it to the layers and save it From a6413c0b83a51cf711d50784ac761cb01d2bc013 Mon Sep 17 00:00:00 2001 From: Rafa de la Torre Date: Thu, 30 Apr 2015 12:56:46 +0200 Subject: [PATCH 2/8] New method User#private_maps_enabled? #3404 Remove details about accounts that should not be the concern of the user model. --- app/models/user.rb | 10 ++++++---- app/models/user/user_decorator.rb | 2 +- app/models/visualization/member.rb | 2 +- spec/models/user_spec.rb | 15 +++++++-------- spec/models/visualization/member_spec.rb | 6 +++--- spec/support/factories/users.rb | 2 +- 6 files changed, 19 insertions(+), 18 deletions(-) diff --git a/app/models/user.rb b/app/models/user.rb index dd3ea76f1e6c..746bb2123af6 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -130,6 +130,7 @@ def before_save end self.max_layers ||= 6 self.private_tables_enabled ||= true + self.private_maps_enabled ||= true self.sync_tables_enabled ||= true end end #before_save @@ -807,10 +808,11 @@ def hard_twitter_datasource_limit=(val) self[:soft_twitter_datasource_limit] = !val end - def private_maps_enabled - enabled = super - return enabled if enabled.present? && enabled == true - /(FREE|MAGELLAN|JOHN SNOW|ACADEMY|ACADEMIC|ON HOLD)/i.match(self.account_type) ? false : true + def private_maps_enabled? + flag_enabled = self.private_maps_enabled + return true if flag_enabled.present? && flag_enabled == true + return true if self.private_tables_enabled # Note private_tables_enabled => private_maps_enabled + return false end def import_quota diff --git a/app/models/user/user_decorator.rb b/app/models/user/user_decorator.rb index 474c4b63d413..24b907e50965 100644 --- a/app/models/user/user_decorator.rb +++ b/app/models/user/user_decorator.rb @@ -52,7 +52,7 @@ def data(options = {}) show_upgraded_message: (self.account_type.downcase != 'free' && self.upgraded_at && self.upgraded_at + 15.days > Date.today ? true : false), actions: { private_tables: self.private_tables_enabled, - private_maps: self.private_maps_enabled, + private_maps: self.private_maps_enabled?, dedicated_support: self.dedicated_support?, import_quota: self.import_quota, remove_logo: self.remove_logo?, diff --git a/app/models/visualization/member.rb b/app/models/visualization/member.rb index f51370d32e21..59b5b144df25 100644 --- a/app/models/visualization/member.rb +++ b/app/models/visualization/member.rb @@ -493,7 +493,7 @@ def get_auth_tokens end def supports_private_maps? - !user.nil? && user.private_maps_enabled + !user.nil? && user.private_maps_enabled? end # @param other_vis CartoDB::Visualization::Member|nil diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 8834d6e23aa1..0ec9c1c22ab5 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -475,26 +475,25 @@ end end - describe '#private_maps_enabled' do + describe '#private_maps_enabled?' do it 'should not have private maps enabled by default' do user_missing_private_maps = create_user :email => 'user_mpm@example.com', :username => 'usermpm', :password => 'usermpm' - user_missing_private_maps.private_maps_enabled.should eq false + user_missing_private_maps.private_maps_enabled?.should eq false end it 'should have private maps if enabled' do user_with_private_maps = create_user :email => 'user_wpm@example.com', :username => 'userwpm', :password => 'userwpm', :private_maps_enabled => true - user_with_private_maps.private_maps_enabled.should eq true + user_with_private_maps.private_maps_enabled?.should eq true end it 'should not have private maps if disabled' do user_without_private_maps = create_user :email => 'user_opm@example.com', :username => 'useropm', :password => 'useropm', :private_maps_enabled => false - user_without_private_maps.private_maps_enabled.should eq false + user_without_private_maps.private_maps_enabled?.should eq false end - it 'should have private maps if he is AMBASSADOR even if disabled' do - user_without_private_maps = create_user :email => 'user_opm2@example.com', :username => 'useropm2', :password => 'useropm2', :private_maps_enabled => false - user_without_private_maps.stubs(:account_type).returns('AMBASSADOR') - user_without_private_maps.private_maps_enabled.should eq true + it 'should have private maps if he has private_tables_enabled even if disabled' do + user_without_private_maps = create_user :email => 'user_opm3@example.com', :username => 'useropm3', :password => 'useropm3', :private_tables_enabled => true + user_without_private_maps.private_maps_enabled?.should eq true end end diff --git a/spec/models/visualization/member_spec.rb b/spec/models/visualization/member_spec.rb index af4d98e2a0bf..471a3d90d2ed 100644 --- a/spec/models/visualization/member_spec.rb +++ b/spec/models/visualization/member_spec.rb @@ -425,7 +425,7 @@ visualization.user_id = user_id # Private maps allowed - @user_mock.stubs(:private_maps_enabled).returns(true) + @user_mock.stubs(:private_maps_enabled?).returns(true) # Forces internal "dirty" flag visualization.privacy = Visualization::Member::PRIVACY_PUBLIC @@ -436,7 +436,7 @@ # ------------- # No private maps allowed - @user_mock.stubs(:private_maps_enabled).returns(false) + @user_mock.stubs(:private_maps_enabled?).returns(false) visualization.privacy = Visualization::Member::PRIVACY_PUBLIC visualization.privacy = Visualization::Member::PRIVACY_PRIVATE @@ -472,7 +472,7 @@ type: Visualization::Member::TYPE_CANONICAL, user_id: user_id ) - @user_mock.stubs(:private_maps_enabled).returns(true) + @user_mock.stubs(:private_maps_enabled?).returns(true) # Careful, do a user mock after touching user_data as it does some checks about user too user_mock = mock diff --git a/spec/support/factories/users.rb b/spec/support/factories/users.rb index 482e4d3f0fba..9296db6488b2 100644 --- a/spec/support/factories/users.rb +++ b/spec/support/factories/users.rb @@ -44,7 +44,7 @@ def new_user(attributes = {}) user.password = attributes[:password] || user.email.split('@').first user.password_confirmation = user.password user.admin = attributes[:admin] == false ? false : true - user.private_tables_enabled = attributes[:private_tables_enabled] == false ? false : true + user.private_tables_enabled = attributes[:private_tables_enabled] == true ? true : false user.private_maps_enabled = attributes[:private_maps_enabled] == true ? true : false user.enabled = attributes[:enabled] == false ? false : true user.table_quota = attributes[:table_quota] if attributes[:table_quota] From a9f501b43ab3a87f19356df0062edd81db1659f4 Mon Sep 17 00:00:00 2001 From: Rafa de la Torre Date: Thu, 30 Apr 2015 14:45:37 +0200 Subject: [PATCH 3/8] BW compat modification to deploy asap #3404 --- app/models/user.rb | 4 ++++ spec/models/user_spec.rb | 10 ++++++++-- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/app/models/user.rb b/app/models/user.rb index 746bb2123af6..01660ce6031d 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -811,6 +811,10 @@ def hard_twitter_datasource_limit=(val) def private_maps_enabled? flag_enabled = self.private_maps_enabled return true if flag_enabled.present? && flag_enabled == true + + #TODO: remove this after making sure we have flags inline with account types + return true if not self.account_type.match(/FREE|MAGELLAN|JOHN SNOW|ACADEMY|ACADEMIC|ON HOLD/i) + return true if self.private_tables_enabled # Note private_tables_enabled => private_maps_enabled return false end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 0ec9c1c22ab5..6318ae42ec52 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -491,8 +491,14 @@ user_without_private_maps.private_maps_enabled?.should eq false end - it 'should have private maps if he has private_tables_enabled even if disabled' do - user_without_private_maps = create_user :email => 'user_opm3@example.com', :username => 'useropm3', :password => 'useropm3', :private_tables_enabled => true + it 'should have private maps if he has private_tables_enabled, even if disabled' do + user_without_private_maps = create_user :email => 'user_opm3@example.com', :username => 'useropm3', :password => 'useropm3', :private_maps_enabled => false, :private_tables_enabled => true + user_without_private_maps.private_maps_enabled?.should eq true + end + + it 'should have private maps if he is AMBASSADOR even if disabled' do + user_without_private_maps = create_user :email => 'user_opm2@example.com', :username => 'useropm2', :password => 'useropm2', :private_maps_enabled => false + user_without_private_maps.stubs(:account_type).returns('AMBASSADOR') user_without_private_maps.private_maps_enabled?.should eq true end From fbbc167e8d5a3590f7cea540c49b08babef35194 Mon Sep 17 00:00:00 2001 From: Rafa de la Torre Date: Thu, 30 Apr 2015 14:57:42 +0200 Subject: [PATCH 4/8] Fix a couple of test that counted on having private_tables_enabled on by default #3404 --- spec/models/layer_spec.rb | 2 +- spec/models/map_spec.rb | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/spec/models/layer_spec.rb b/spec/models/layer_spec.rb index 3d2efdca1c73..aff1bd2421c2 100644 --- a/spec/models/layer_spec.rb +++ b/spec/models/layer_spec.rb @@ -5,7 +5,7 @@ before(:all) do @quota_in_bytes = 500.megabytes @table_quota = 500 - @user = create_user(:quota_in_bytes => @quota_in_bytes, :table_quota => @table_quota) + @user = create_user(:quota_in_bytes => @quota_in_bytes, :table_quota => @table_quota, :private_tables_enabled => true) end after(:all) do diff --git a/spec/models/map_spec.rb b/spec/models/map_spec.rb index 4583da29dbca..568a8f002b21 100644 --- a/spec/models/map_spec.rb +++ b/spec/models/map_spec.rb @@ -9,7 +9,8 @@ @table_quota = 500 @user = create_user( quota_in_bytes: @quota_in_bytes, - table_quota: @table_quota + table_quota: @table_quota, + private_tables_enabled: true ) end From 433aaa45f0d7d4cf54515c3cef7169a977dc563d Mon Sep 17 00:00:00 2001 From: Rafa de la Torre Date: Thu, 30 Apr 2015 15:40:40 +0200 Subject: [PATCH 5/8] Fix tests by granting private_tables to user #3404 (as previously done in factory by default) --- spec/models/named_maps_spec.rb | 2 +- spec/models/visualization/collection_spec.rb | 11 +++++------ 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/spec/models/named_maps_spec.rb b/spec/models/named_maps_spec.rb index fd42d43ddeb9..7ad837bca64d 100644 --- a/spec/models/named_maps_spec.rb +++ b/spec/models/named_maps_spec.rb @@ -43,7 +43,7 @@ def named_map_url(template_id, user_api_key) Visualization.repository = DataRepository::Backend::Sequel.new(Rails::Sequel.connection, :visualizations) puts "\n[rspec][#{SPEC_NAME}] Creating test user database..." - @user = create_user( :quota_in_bytes => 524288000, :table_quota => 100 ) + @user = create_user( :quota_in_bytes => 524288000, :table_quota => 100, :private_tables_enabled => true ) puts "[rspec][#{SPEC_NAME}] Running..." end diff --git a/spec/models/visualization/collection_spec.rb b/spec/models/visualization/collection_spec.rb index 2f4beac4a81a..bc7123fc3d75 100644 --- a/spec/models/visualization/collection_spec.rb +++ b/spec/models/visualization/collection_spec.rb @@ -400,11 +400,10 @@ def restore_vis_backend_to_normal_table_so_relator_works it 'counts total liked' do restore_vis_backend_to_normal_table_so_relator_works - user1 = create_user(:quota_in_bytes => 524288000, :table_quota => 500) + user1 = create_user(:quota_in_bytes => 524288000, :table_quota => 500, :private_tables_enabled => true) user1.stubs(:organization).returns(nil) - user2 = create_user(:quota_in_bytes => 524288000, :table_quota => 500) - user2.stubs(:private_tables_enabled).returns(true) + user2 = create_user(:quota_in_bytes => 524288000, :table_quota => 500, :private_tables_enabled => true) user2.stubs(:organization).returns(nil) table11 = create_table(user1) @@ -507,9 +506,9 @@ def restore_vis_backend_to_normal_table_so_relator_works it "checks filtering by 'liked' " do restore_vis_backend_to_normal_table_so_relator_works - user = create_user(:quota_in_bytes => 524288000, :table_quota => 500) - user2 = create_user(:quota_in_bytes => 524288000, :table_quota => 500) - user3 = create_user(:quota_in_bytes => 524288000, :table_quota => 500) + user = create_user(:quota_in_bytes => 524288000, :table_quota => 500, :private_tables_enabled => true) + user2 = create_user(:quota_in_bytes => 524288000, :table_quota => 500, :private_tables_enabled => true) + user3 = create_user(:quota_in_bytes => 524288000, :table_quota => 500, :private_tables_enabled => true) CartoDB::Visualization::Relator.any_instance.stubs(:user).returns(user) table1 = Table.new From ecc8bfceab6fd25a4a527dc1d66bd32a8b245911 Mon Sep 17 00:00:00 2001 From: Rafa de la Torre Date: Thu, 30 Apr 2015 16:02:59 +0200 Subject: [PATCH 6/8] Fix tests by granting private_tables to users (as prev done in factory by default) #3404 --- spec/requests/admin/visualizations_spec.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/spec/requests/admin/visualizations_spec.rb b/spec/requests/admin/visualizations_spec.rb index be9109e4bfca..47abd2617f2a 100644 --- a/spec/requests/admin/visualizations_spec.rb +++ b/spec/requests/admin/visualizations_spec.rb @@ -22,7 +22,8 @@ def app @user = create_user( username: 'test', email: 'test@test.com', - password: 'test12' + password: 'test12', + private_table_enabled: true ) @api_key = @user.api_key @user.stubs(:should_load_common_data?).returns(false) From d6b2fe29151732552f187a59949309a943ba7d6d Mon Sep 17 00:00:00 2001 From: Rafa de la Torre Date: Thu, 30 Apr 2015 17:03:08 +0200 Subject: [PATCH 7/8] Fix tests by granting private_tables to users (as prev done in factory by default) #3404 --- spec/requests/admin/visualizations_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/requests/admin/visualizations_spec.rb b/spec/requests/admin/visualizations_spec.rb index 47abd2617f2a..3a964d276369 100644 --- a/spec/requests/admin/visualizations_spec.rb +++ b/spec/requests/admin/visualizations_spec.rb @@ -23,7 +23,7 @@ def app username: 'test', email: 'test@test.com', password: 'test12', - private_table_enabled: true + private_tables_enabled: true ) @api_key = @user.api_key @user.stubs(:should_load_common_data?).returns(false) From 751fae670925f496904477bb460b31aad4486519 Mon Sep 17 00:00:00 2001 From: Rafa de la Torre Date: Thu, 30 Apr 2015 17:14:38 +0200 Subject: [PATCH 8/8] Fix tests by granting private_tables to users (as prev done in factory by default) #3404 --- spec/requests/api/tables_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/requests/api/tables_spec.rb b/spec/requests/api/tables_spec.rb index 0ede2f59ef28..6fbf861742ff 100644 --- a/spec/requests/api/tables_spec.rb +++ b/spec/requests/api/tables_spec.rb @@ -6,7 +6,7 @@ before(:all) do CartoDB::Varnish.any_instance.stubs(:send_command).returns(true) - @user = create_user(:username => 'test', :email => "client@example.com", :password => "clientex") + @user = create_user(:username => 'test', :email => "client@example.com", :password => "clientex", :private_tables_enabled => true) @another_user = create_user end