From 944c72191d536f873286e3f03214d1ede0b910da Mon Sep 17 00:00:00 2001 From: Jorge Tarrero Date: Thu, 12 Nov 2020 17:23:05 +0100 Subject: [PATCH 1/9] Email notifications toggle API endpoint --- .rubocop.yml | 9 ++ .../api/email_notifications_controller.rb | 41 +++++++++ app/models/carto/user.rb | 23 +++++ app/models/carto/user_email_notification.rb | 27 ++++++ config/routes.rb | 3 + ...1102307_create_user_email_notifications.rb | 20 ++++ spec/models/carto/user_spec.rb | 91 +++++++++++++++++++ .../email_notifications_controller_spec.rb | 52 +++++++++++ 8 files changed, 266 insertions(+) create mode 100644 app/controllers/carto/api/email_notifications_controller.rb create mode 100644 app/models/carto/user_email_notification.rb create mode 100644 db/migrate/20201111102307_create_user_email_notifications.rb create mode 100644 spec/requests/carto/api/email_notifications_controller_spec.rb diff --git a/.rubocop.yml b/.rubocop.yml index e851208b0f..e4c9060d4a 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -153,3 +153,12 @@ Style/SymbolArray: Style/WordArray: Description: Use %w or %W for arrays of words. Enabled: false + + +#################### +# Rails +#################### + +Rails/CreateTableWithTimestamps: + Description: Create new tables with created_at and updated_at timestamps + Enabled: false diff --git a/app/controllers/carto/api/email_notifications_controller.rb b/app/controllers/carto/api/email_notifications_controller.rb new file mode 100644 index 0000000000..4ebd36f35d --- /dev/null +++ b/app/controllers/carto/api/email_notifications_controller.rb @@ -0,0 +1,41 @@ +require_dependency 'carto/uuidhelper' + +module Carto + module Api + class EmailNotificationsController < ::Api::ApplicationController + + ssl_required :update + before_action :load_notifications, only: [:show, :update] + + rescue_from StandardError, with: :rescue_from_standard_error + rescue_from Carto::LoadError, with: :rescue_from_carto_error + + def show + render_jsonp({ notifications: decorate_notifications }, 200) + end + + def update + notifications = params.require(:notifications) + @carto_user.email_notifications = notifications + + render_jsonp({}, 204) + end + + private + + def load_notifications + @carto_user = Carto::User.find(current_user.id) + @notifications = @carto_user.email_notifications + end + + def decorate_notifications + payload = {} + @notifications.each do |notification| + payload[notification.notification] = notification.enabled + end + payload + end + + end + end +end diff --git a/app/models/carto/user.rb b/app/models/carto/user.rb index dd395919f5..d7d9d9a6df 100644 --- a/app/models/carto/user.rb +++ b/app/models/carto/user.rb @@ -38,6 +38,7 @@ class Carto::User < ActiveRecord::Base belongs_to :rate_limit has_one :owned_organization, class_name: Carto::Organization, inverse_of: :owner, foreign_key: :owner_id has_one :static_notifications, class_name: Carto::UserNotification, inverse_of: :user + has_many :email_notifications, class_name: Carto::UserEmailNotification, inverse_of: :user, dependent: :destroy has_many :self_feature_flags_user, dependent: :destroy, foreign_key: :user_id, inverse_of: :user, class_name: Carto::FeatureFlagsUser has_many :self_feature_flags, through: :self_feature_flags_user, source: :feature_flag @@ -293,6 +294,28 @@ class Carto::User < ActiveRecord::Base reload.dbdirect_ip end + # @param [Hash] notifications_hash A notification list with this format: {"notif_a" => true, "notif_b" => false} + def email_notifications=(notifications_hash) + notifications_hash.each do |key, value| + Carto::UserEmailNotification + .find_or_initialize_by(user_id: id, notification: key) + .update!(enabled: value) + end + reload + rescue StandardError => e + log_warning(message: 'Tried to create an invalid notification', exception: e, current_user: self) + raise e + end + + def email_notification_enabled?(notification) + # By default, the email notifications are enabled if the row is missing. + email_notification = email_notifications.find { |n| n.notification == notification } + + return true if email_notification.nil? + + email_notification.enabled + end + private def set_database_host diff --git a/app/models/carto/user_email_notification.rb b/app/models/carto/user_email_notification.rb new file mode 100644 index 0000000000..cf801608ab --- /dev/null +++ b/app/models/carto/user_email_notification.rb @@ -0,0 +1,27 @@ +module Carto + class UserEmailNotification < ActiveRecord::Base + + belongs_to :user, class_name: 'Carto::User', inverse_of: :email_notifications + + validates :user, presence: true + validate :valid_notification + + NOTIFICATION_DO_SUBSCRIPTIONS = 'do_subscriptions'.freeze + VALID_NOTIFICATIONS = [NOTIFICATION_DO_SUBSCRIPTIONS].freeze + + def self.new_do_subscriptions(user_id) + Carto::UserEmailNotification.create!( + user_id: user_id, + notification: NOTIFICATION_DO_SUBSCRIPTIONS, + enabled: true + ) + end + + private + + def valid_notification + errors.add(:notification, 'Invalid notification') unless VALID_NOTIFICATIONS.include?(notification) + end + + end +end diff --git a/config/routes.rb b/config/routes.rb index 4403d1239a..bd0be87db2 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -689,6 +689,9 @@ CartoDB::Application.routes.draw do put 'notifications/:category', to: 'static_notifications#update', as: :api_v3_static_notifications_update + get 'email_notifications', to: 'email_notifications#show', as: :api_v3_email_notifications_show + put 'email_notifications', to: 'email_notifications#update', as: :api_v3_email_notifications_update + resources :organizations, only: [] do resources :notifications, only: [:create, :destroy], controller: :organization_notifications, diff --git a/db/migrate/20201111102307_create_user_email_notifications.rb b/db/migrate/20201111102307_create_user_email_notifications.rb new file mode 100644 index 0000000000..9414f52a3a --- /dev/null +++ b/db/migrate/20201111102307_create_user_email_notifications.rb @@ -0,0 +1,20 @@ +require 'carto/db/migration_helper' + +include Carto::Db::MigrationHelper + +migration( + proc do + create_table :user_email_notifications do + Uuid :id, primary_key: true, default: Sequel.lit('uuid_generate_v4()') + foreign_key :user_id, :users, type: :uuid, null: false, on_delete: :cascade + String :notification, null: false + Boolean :enabled + DateTime :created_at, null: false + DateTime :updated_at, null: false + index [:user_id, :notification], unique: true + end + end, + proc do + drop_table :user_email_notifications + end +) diff --git a/spec/models/carto/user_spec.rb b/spec/models/carto/user_spec.rb index ee55a06b19..91fca784d4 100644 --- a/spec/models/carto/user_spec.rb +++ b/spec/models/carto/user_spec.rb @@ -101,4 +101,95 @@ describe Carto::User do @user.password_reset_sent_at.to_s.should eql now.to_s end end + + describe "#is_email_notification_enabled" do + before(:all) do + @carto_user = FactoryGirl.create(:carto_user) + end + + it 'returns the notification flag if it exists' do + notification_type = Carto::UserEmailNotification::NOTIFICATION_DO_SUBSCRIPTIONS + email_notification = Carto::UserEmailNotification.create( + user_id: @carto_user.id, + notification: notification_type, + enabled: false + ) + + expect(@carto_user.email_notification_enabled?(notification_type)).to be_false + email_notification.destroy + end + + it 'returns true as default if notification is missing' do + expect(@carto_user.email_notification_enabled?('missing')).to be_true + end + + it 'cascade delete notifications if the user is destroyed' do + notification_type = Carto::UserEmailNotification::NOTIFICATION_DO_SUBSCRIPTIONS + email_notification = Carto::UserEmailNotification.create( + user_id: @carto_user.id, + notification: notification_type, + enabled: true + ) + + @carto_user.destroy + expect do + Carto::UserEmailNotification.find(email_notification.id) + end.to raise_error ActiveRecord::RecordNotFound + end + + it 'does not create object if an invalid type is provided' do + email_notification = Carto::UserEmailNotification.new( + user_id: @carto_user.id, + notification: 'invalid_type', + enabled: true + ) + expect(email_notification.valid?).to be_false + end + end + + describe '#email_notification=' do + before(:all) do + @carto_user = FactoryGirl.create(:carto_user) + end + + it 'creates a fresh set of notifications' do + Carto::UserEmailNotification.any_instance.stubs(:valid_notification).returns(true) + @carto_user.email_notifications = { + notif_a: true, + notif_b: false + } + + expect(@carto_user.email_notifications.length).to eq 2 + expect(@carto_user.email_notification_enabled?('notif_a')).to be_true + expect(@carto_user.email_notification_enabled?('notif_b')).to be_false + end + + it 'updates notifications if they already exist' do + Carto::UserEmailNotification.any_instance.stubs(:valid_notification).returns(true) + Carto::UserEmailNotification.create( + user_id: @carto_user.id, + notification: 'existing_notification', + enabled: true + ) + + @carto_user.email_notifications = { + notif_a: true, + notif_b: false, + existing_notification: false + } + + expect(@carto_user.email_notifications.length).to eq 3 + expect(@carto_user.email_notification_enabled?('notif_a')).to be_true + expect(@carto_user.email_notification_enabled?('notif_b')).to be_false + expect(@carto_user.email_notification_enabled?('existing_notification')).to be_false + end + + it 'raises an error if an invalid notification is set' do + expect do + @carto_user.email_notifications = { + notif_a: true + } + end.to raise_error StandardError + end + end end diff --git a/spec/requests/carto/api/email_notifications_controller_spec.rb b/spec/requests/carto/api/email_notifications_controller_spec.rb new file mode 100644 index 0000000000..a9a1d0cc5b --- /dev/null +++ b/spec/requests/carto/api/email_notifications_controller_spec.rb @@ -0,0 +1,52 @@ +require 'spec_helper_min' +require 'support/helpers' + +describe Carto::Api::EmailNotificationsController do + include HelperMethods + + before(:all) do + @carto_user = FactoryGirl.create(:carto_user) + @carto_user.email_notifications = { + do_subscriptions: true + } + end + + let(:auth_params) do + { user_domain: @carto_user.username, api_key: @carto_user.api_key } + end + + describe '#show' do + it 'list the current notifications' do + get_json(api_v3_email_notifications_show_url(auth_params)) do |response| + response.status.should eq 200 + response.body.should eq({ notifications: { do_subscriptions: true } }) + end + end + + it 'return error if unauthenticated' do + get_json(api_v3_email_notifications_show_url({})) do |response| + response.status.should eq 401 + end + end + end + + describe '#update' do + it 'return error if unauthenticated' do + put_json(api_v3_email_notifications_update_url({})) do |response| + response.status.should eq 401 + end + end + + it 'returns error if an invalid notification is provided' do + put_json(api_v3_email_notifications_update_url(auth_params), { notifications: { invalid: true } }) do |response| + response.status.should eq 500 + end + end + + it 'successfully updates notifications' do + put_json(api_v3_email_notifications_update_url(auth_params), { notifications: { do_subscriptions: false } }) do |response| + response.status.should eq 204 + end + end + end +end From 8c6de9e7844aa543254f3c46745ac9ce4f313c17 Mon Sep 17 00:00:00 2001 From: Jorge Tarrero Date: Fri, 13 Nov 2020 09:28:25 +0100 Subject: [PATCH 2/9] Update NEWS --- NEWS.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/NEWS.md b/NEWS.md index fbe870614a..4a727e5caf 100644 --- a/NEWS.md +++ b/NEWS.md @@ -5,7 +5,7 @@ Development - None yet ### Features -- None yet +* Email notifications toggle API endpoint [#15930](https://github.com/CartoDB/cartodb/pull/15930) ### Bug fixes / enhancements * Fix BigQuery connector not importing 0-bytes-processed datasets [#15916](https://github.com/CartoDB/cartodb/pull/15916) From aa52aff349d6213b4e86d0cff79ad734d2f1ee77 Mon Sep 17 00:00:00 2001 From: Jorge Tarrero Date: Fri, 13 Nov 2020 11:17:19 +0100 Subject: [PATCH 3/9] Fix linter --- app/models/carto/user.rb | 2 +- spec/models/carto/user_spec.rb | 2 +- spec/requests/carto/api/email_notifications_controller_spec.rb | 3 ++- 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/app/models/carto/user.rb b/app/models/carto/user.rb index d7d9d9a6df..a025c8a6b8 100644 --- a/app/models/carto/user.rb +++ b/app/models/carto/user.rb @@ -38,7 +38,7 @@ class Carto::User < ActiveRecord::Base belongs_to :rate_limit has_one :owned_organization, class_name: Carto::Organization, inverse_of: :owner, foreign_key: :owner_id has_one :static_notifications, class_name: Carto::UserNotification, inverse_of: :user - has_many :email_notifications, class_name: Carto::UserEmailNotification, inverse_of: :user, dependent: :destroy + has_many :email_notifications, class_name: 'Carto::UserEmailNotification', inverse_of: :user, dependent: :destroy has_many :self_feature_flags_user, dependent: :destroy, foreign_key: :user_id, inverse_of: :user, class_name: Carto::FeatureFlagsUser has_many :self_feature_flags, through: :self_feature_flags_user, source: :feature_flag diff --git a/spec/models/carto/user_spec.rb b/spec/models/carto/user_spec.rb index 91fca784d4..f56facb70f 100644 --- a/spec/models/carto/user_spec.rb +++ b/spec/models/carto/user_spec.rb @@ -102,7 +102,7 @@ describe Carto::User do end end - describe "#is_email_notification_enabled" do + describe '#is_email_notification_enabled' do before(:all) do @carto_user = FactoryGirl.create(:carto_user) end diff --git a/spec/requests/carto/api/email_notifications_controller_spec.rb b/spec/requests/carto/api/email_notifications_controller_spec.rb index a9a1d0cc5b..6b15bfe5b7 100644 --- a/spec/requests/carto/api/email_notifications_controller_spec.rb +++ b/spec/requests/carto/api/email_notifications_controller_spec.rb @@ -44,7 +44,8 @@ describe Carto::Api::EmailNotificationsController do end it 'successfully updates notifications' do - put_json(api_v3_email_notifications_update_url(auth_params), { notifications: { do_subscriptions: false } }) do |response| + params = { notifications: { do_subscriptions: false } } + put_json(api_v3_email_notifications_update_url(auth_params), params) do |response| response.status.should eq 204 end end From de47b8eed845dd89a6cb54ba8c467229aa6d3991 Mon Sep 17 00:00:00 2001 From: Jorge Tarrero Date: Mon, 16 Nov 2020 10:58:47 +0100 Subject: [PATCH 4/9] Apply CR suggestions --- app/controllers/carto/api/email_notifications_controller.rb | 3 --- 1 file changed, 3 deletions(-) diff --git a/app/controllers/carto/api/email_notifications_controller.rb b/app/controllers/carto/api/email_notifications_controller.rb index 4ebd36f35d..44b1d1b5c2 100644 --- a/app/controllers/carto/api/email_notifications_controller.rb +++ b/app/controllers/carto/api/email_notifications_controller.rb @@ -1,5 +1,3 @@ -require_dependency 'carto/uuidhelper' - module Carto module Api class EmailNotificationsController < ::Api::ApplicationController @@ -8,7 +6,6 @@ module Carto before_action :load_notifications, only: [:show, :update] rescue_from StandardError, with: :rescue_from_standard_error - rescue_from Carto::LoadError, with: :rescue_from_carto_error def show render_jsonp({ notifications: decorate_notifications }, 200) From 14c6a4a35c3590a3efc94004f355f4c8a0b24f4d Mon Sep 17 00:00:00 2001 From: Jorge Tarrero Date: Tue, 17 Nov 2020 11:24:03 +0100 Subject: [PATCH 5/9] Apply CR suggestions --- app/controllers/carto/api/email_notifications_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/carto/api/email_notifications_controller.rb b/app/controllers/carto/api/email_notifications_controller.rb index 44b1d1b5c2..8b84c6a53f 100644 --- a/app/controllers/carto/api/email_notifications_controller.rb +++ b/app/controllers/carto/api/email_notifications_controller.rb @@ -3,7 +3,7 @@ module Carto class EmailNotificationsController < ::Api::ApplicationController ssl_required :update - before_action :load_notifications, only: [:show, :update] + before_action :load_notifications rescue_from StandardError, with: :rescue_from_standard_error From 9dc4f6775c04d4007c4e4de4c076cc4a3d4e0f71 Mon Sep 17 00:00:00 2001 From: Jorge Tarrero Date: Tue, 17 Nov 2020 17:25:31 +0100 Subject: [PATCH 6/9] Make ::UserTable destroy ORM callback use the proper Table service --- app/models/table/user_table.rb | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/app/models/table/user_table.rb b/app/models/table/user_table.rb index 7c12bd2616..775b654a9f 100644 --- a/app/models/table/user_table.rb +++ b/app/models/table/user_table.rb @@ -201,7 +201,12 @@ class UserTable < Sequel::Model end def destroy - Carto::UserTable.find_by(id: id)&.destroy + carto_user_table = Carto::UserTable.find_by(id: id) + + return if carto_user_table.nil? + + carto_user_table.set_service(service) + carto_user_table.destroy end def before_update From 81e0334153d0e8320a4d01b79919d640185f4814 Mon Sep 17 00:00:00 2001 From: Jorge Tarrero Date: Tue, 17 Nov 2020 17:28:14 +0100 Subject: [PATCH 7/9] Update NEWS --- NEWS.md | 1 + 1 file changed, 1 insertion(+) diff --git a/NEWS.md b/NEWS.md index 4fc301c052..191bbc9f5e 100644 --- a/NEWS.md +++ b/NEWS.md @@ -11,6 +11,7 @@ Development * Fix BigQuery connector not importing 0-bytes-processed datasets [#15916](https://github.com/CartoDB/cartodb/pull/15916) * Error importing geopackage files with multiple layers [#15907](https://github.com/CartoDB/cartodb/pull/15907) * Add DO notification in dashboard [#15929](https://github.com/CartoDB/cartodb/pull/15929) +* Data loss on table rename due to GhostTablesManager 4.43.0 (2020-11-06) ------------------- From ade4b51b652a66ed011166dc6a1b865bf990756b Mon Sep 17 00:00:00 2001 From: Jorge Tarrero Date: Tue, 17 Nov 2020 17:49:16 +0100 Subject: [PATCH 8/9] Dummy commit to trigger checks --- app/models/table/user_table.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/app/models/table/user_table.rb b/app/models/table/user_table.rb index 775b654a9f..fc1202f2e0 100644 --- a/app/models/table/user_table.rb +++ b/app/models/table/user_table.rb @@ -201,12 +201,12 @@ class UserTable < Sequel::Model end def destroy - carto_user_table = Carto::UserTable.find_by(id: id) + ar_user_table = Carto::UserTable.find_by(id: id) - return if carto_user_table.nil? + return if ar_user_table.nil? - carto_user_table.set_service(service) - carto_user_table.destroy + ar_user_table.set_service(service) + ar_user_table.destroy end def before_update From 24f7b121a1621f5c87d30a020803bcb9a571b72e Mon Sep 17 00:00:00 2001 From: Jorge Tarrero Date: Tue, 17 Nov 2020 18:08:11 +0100 Subject: [PATCH 9/9] Update NEWS --- NEWS.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/NEWS.md b/NEWS.md index 191bbc9f5e..41b6ffb668 100644 --- a/NEWS.md +++ b/NEWS.md @@ -11,7 +11,7 @@ Development * Fix BigQuery connector not importing 0-bytes-processed datasets [#15916](https://github.com/CartoDB/cartodb/pull/15916) * Error importing geopackage files with multiple layers [#15907](https://github.com/CartoDB/cartodb/pull/15907) * Add DO notification in dashboard [#15929](https://github.com/CartoDB/cartodb/pull/15929) -* Data loss on table rename due to GhostTablesManager +* Data loss on table rename due to GhostTablesManager [#15935](https://github.com/CartoDB/cartodb/pull/15935) 4.43.0 (2020-11-06) -------------------