From cc34f333ac284b011c9b1d05dc88c990e4e40731 Mon Sep 17 00:00:00 2001 From: "Dan A. Lipeles" Date: Tue, 21 May 2019 13:56:10 -0400 Subject: [PATCH 1/5] =?UTF-8?q?Site=20Banner=20Fix=20=F0=9F=8F=B7=20=20(#1?= =?UTF-8?q?892)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- app/models/site_banner.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/models/site_banner.rb b/app/models/site_banner.rb index 13a1ee5e11..fb07b4b7ad 100644 --- a/app/models/site_banner.rb +++ b/app/models/site_banner.rb @@ -38,15 +38,15 @@ def clear_invalid_social_links require 'addressable' self.social_links = social_links.select { |key, _| key.in?(["twitch", "youtube", "twitter"]) } - unless social_links["twitch"].blank? || Addressable::URI.parse(social_links["twitch"]).normalize.host.in?(["www.twitch.tv", "twitch.tv"]) + unless social_links["twitch"].blank? || Addressable::URI.parse(social_links["twitch"]).to_s.starts_with?('https://www.twitch.tv/', 'https://twitch.tv/', 'www.twitch.tv/', 'twitch.tv/') social_links["twitch"] = "" end - unless social_links["youtube"].blank? || Addressable::URI.parse(social_links["youtube"]).normalize.host.in?(["www.youtube.com", "youtube.com"]) + unless social_links["youtube"].blank? || Addressable::URI.parse(social_links["youtube"]).to_s.starts_with?('https://www.youtube.com/', 'https://youtube.com/', 'www.youtube.com/', 'youtube.com/') social_links["youtube"] = "" end - unless social_links["twitter"].blank? || Addressable::URI.parse(social_links["twitter"]).normalize.host.in?(["www.twitter.com", "twitter.com"]) + unless social_links["twitter"].blank? || Addressable::URI.parse(social_links["twitter"]).to_s.starts_with?('https://www.twitter.com/', 'https://twitter.com/', 'www.twitter.com/', 'twitter.com/') social_links["twitter"] = "" end end From 0df1cc599500e74d713a6f1fba1347c73731ca9a Mon Sep 17 00:00:00 2001 From: "Dan A. Lipeles" Date: Tue, 21 May 2019 21:37:56 -0400 Subject: [PATCH 2/5] =?UTF-8?q?Enable=20Twitter=20Channels=20=F0=9F=8F=B7?= =?UTF-8?q?=20=20(#1897)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../publishers/_choose_channel_type.html.slim | 17 ++++++++--------- config/locales/en.yml | 2 +- 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/app/views/publishers/_choose_channel_type.html.slim b/app/views/publishers/_choose_channel_type.html.slim index bef823edb1..77b17c4bb4 100644 --- a/app/views/publishers/_choose_channel_type.html.slim +++ b/app/views/publishers/_choose_channel_type.html.slim @@ -29,15 +29,14 @@ h6= t "publishers.choose_new_channel_type.twitch.heading" = t "publishers.choose_new_channel_type.twitch.description" -- if params[:twitter] - = link_to(publisher_register_twitter_channel_omniauth_authorize_path, class: 'card block-link--card', :"data-piwik-action" => "AddTwitterChannelClicked", :"data-piwik-name" => "Clicked", :"data-piwik-value" => "AddChannelFlow") do - .card-body - .icon-and-text--wrapper - .icon-and-text--icon - = image_tag "choose-channel/choose-channel--twitter.jpg", height: 72, width: 88, class: "choose-channel--icon" - .icon-and-text--text - h6= t "publishers.choose_new_channel_type.twitter.heading" - = t "publishers.choose_new_channel_type.twitter.description" += link_to(publisher_register_twitter_channel_omniauth_authorize_path, class: 'card block-link--card', :"data-piwik-action" => "AddTwitterChannelClicked", :"data-piwik-name" => "Clicked", :"data-piwik-value" => "AddChannelFlow") do + .card-body + .icon-and-text--wrapper + .icon-and-text--icon + = image_tag "choose-channel/choose-channel--twitter.jpg", height: 72, width: 88, class: "choose-channel--icon" + .icon-and-text--text + h6= t "publishers.choose_new_channel_type.twitter.heading" + = t "publishers.choose_new_channel_type.twitter.description" - if params[:vimeo] = link_to(publisher_register_vimeo_channel_omniauth_authorize_path, class: 'card block-link--card', :"data-piwik-action" => "AddVimeoChannelClicked", :"data-piwik-name" => "Clicked", :"data-piwik-value" => "AddChannelFlow") do diff --git a/config/locales/en.yml b/config/locales/en.yml index 271b923f4f..efa9bc0d1c 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -369,7 +369,7 @@ en: description: I'm the primary owner of a Twitch channel. twitter: heading: TWITTER - description: I own a Twitter account. + description: I own or manage a Twitter account. vimeo: heading: VIMEO CHANNEL description: I own or manage a Vimeo channel. From b8602389da8585a22e371031a2c63216ccad2be6 Mon Sep 17 00:00:00 2001 From: Cory McDonald Date: Thu, 23 May 2019 10:29:31 -0500 Subject: [PATCH 3/5] Add twitter to the statistical totals (#1908) --- app/models/channel.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/models/channel.rb b/app/models/channel.rb index 01be60977c..a08af50da0 100644 --- a/app/models/channel.rb +++ b/app/models/channel.rb @@ -138,6 +138,7 @@ def self.statistical_totals twitch: Channel.verified.twitch_channels.count, youtube: Channel.verified.youtube_channels.count, site: Channel.verified.site_channels.count, + twitter: Channel.verified.twitter_channels.count, } end From b8ed4ef2725a1df29f3c1b7c38c4e788f12ed2d0 Mon Sep 17 00:00:00 2001 From: Cory McDonald Date: Thu, 23 May 2019 11:03:16 -0500 Subject: [PATCH 4/5] Create totals for new channels (#1909) * Create totals for new channels * add vimeo --- app/models/channel.rb | 3 +++ 1 file changed, 3 insertions(+) diff --git a/app/models/channel.rb b/app/models/channel.rb index a08af50da0..af7bfc75dd 100644 --- a/app/models/channel.rb +++ b/app/models/channel.rb @@ -139,6 +139,9 @@ def self.statistical_totals youtube: Channel.verified.youtube_channels.count, site: Channel.verified.site_channels.count, twitter: Channel.verified.twitter_channels.count, + reddit: 0, + github: 0, + vimeo: Channel.verified.vimeo_channels.count, } end From 9002dedd7109b424b58625afa5b547a316bf9dac Mon Sep 17 00:00:00 2001 From: Albert Wang Date: Thu, 23 May 2019 14:33:59 -0700 Subject: [PATCH 5/5] Feature/deleted account (#1843) * Logic for deletion * Test for removing a publisher * Test against versions * Allow admins to delete * Allow admin to delete users. Closes #751 * Performance and readability improvements * Performance improvements and bugfix for publisher test * Change the way we save deleted ip addresses * Add deleted state back in --- .../admin/publishers_controller.rb | 7 ++ app/jobs/publisher_removal_job.rb | 26 +++++++ app/models/publisher.rb | 18 +++-- app/models/publisher_status_update.rb | 3 +- app/views/admin/publishers/edit.html.slim | 2 + test/jobs/publisher_removal_job_test.rb | 18 +++++ test/models/publisher_status_update_test.rb | 3 +- test/models/publisher_test.rb | 75 ++++++++++--------- 8 files changed, 106 insertions(+), 46 deletions(-) create mode 100644 app/jobs/publisher_removal_job.rb create mode 100644 test/jobs/publisher_removal_job_test.rb diff --git a/app/controllers/admin/publishers_controller.rb b/app/controllers/admin/publishers_controller.rb index 65b5da2cab..c68671b07c 100644 --- a/app/controllers/admin/publishers_controller.rb +++ b/app/controllers/admin/publishers_controller.rb @@ -22,6 +22,7 @@ def index if params[:two_factor_authentication_removal].present? @publishers = @publishers.joins(:two_factor_authentication_removal).distinct end + @publishers = @publishers.where.not(email: nil).or(@publishers.where.not(pending_email: nil)) # Don't include deleted users @publishers = @publishers.group(:id).paginate(page: params[:page]) end @@ -43,6 +44,12 @@ def update redirect_to admin_publisher_path(@publisher) end + def destroy + PublisherRemovalJob.perform_later(publisher_id: @publisher.id) + flash[:alert] = "Deletion job enqueued. This usually takes a few seconds to complete" + redirect_to admin_publisher_path(@publisher) + end + def statement statement_period = params[:statement_period] @transactions = PublisherStatementGetter.new(publisher: @publisher, statement_period: statement_period).perform diff --git a/app/jobs/publisher_removal_job.rb b/app/jobs/publisher_removal_job.rb new file mode 100644 index 0000000000..0c7fe96107 --- /dev/null +++ b/app/jobs/publisher_removal_job.rb @@ -0,0 +1,26 @@ +class PublisherRemovalJob < ApplicationJob + queue_as :default + + def perform(publisher_id:) + publisher = Publisher.find_by(id: publisher_id) + return if publisher.suspended? + publisher.status_updates.create(status: PublisherStatusUpdate::DELETED) + ActiveRecord::Base.transaction do + publisher.update(email: nil) + publisher.update(pending_email: nil) + publisher.update(name: PublisherStatusUpdate::DELETED) + publisher.update(phone: nil) + publisher.update(phone_normalized: nil) + # If they're signed in, they should not longer be signed in + publisher.user_authentication_token.update(authentication_token_expires_at: Time.now) if publisher.user_authentication_token.present? + publisher.update(current_sign_in_ip: nil) + publisher.update(last_sign_in_ip: nil) + end + publisher.channels.pluck(:id).each do |channel_id| + DeletePublisherChannelJob.perform_now(channel_id: channel_id) + end + + # Paper trail retains all records: we destroy all historical PII and non-PII + publisher.versions.destroy_all + end +end diff --git a/app/models/publisher.rb b/app/models/publisher.rb index 50ec4cd300..fb4f4eda2e 100644 --- a/app/models/publisher.rb +++ b/app/models/publisher.rb @@ -44,12 +44,12 @@ class Publisher < ApplicationRecord # Normalizes attribute before validation and saves into other attribute phony_normalize :phone, as: :phone_normalized, default_country_code: "US" - validates :email, email: { strict_mode: true }, presence: true, unless: -> { pending_email.present? } - validates :email, uniqueness: { case_sensitive: false }, allow_nil: true - validates :pending_email, email: { strict_mode: true }, presence: true, if: -> { email.blank? } + validates :email, email: { strict_mode: true }, presence: true, unless: -> { pending_email.present? || deleted? } + validates :email, uniqueness: { case_sensitive: false }, allow_nil: true, unless: -> { deleted? } + validates :pending_email, email: { strict_mode: true }, presence: true, if: -> { email.blank? && !deleted? } validates :promo_registrations, length: { maximum: MAX_PROMO_REGISTRATIONS } - validate :pending_email_must_be_a_change - validate :pending_email_can_not_be_in_use + validate :pending_email_must_be_a_change, unless: -> { deleted? } + validate :pending_email_can_not_be_in_use, unless: -> { deleted? } validates :name, presence: true, allow_blank: true validates :phone_normalized, phony_plausible: true @@ -179,8 +179,12 @@ def history combined.map { |c| c.values.first } end + def deleted? + last_status_update&.status == PublisherStatusUpdate::DELETED + end + def suspended? - last_status_update.present? && last_status_update.status == PublisherStatusUpdate::SUSPENDED + last_status_update&.status == PublisherStatusUpdate::SUSPENDED end def no_grants? @@ -188,7 +192,7 @@ def no_grants? end def locked? - last_status_update.present? && last_status_update.status == PublisherStatusUpdate::LOCKED + last_status_update&.status == PublisherStatusUpdate::LOCKED end def verified? diff --git a/app/models/publisher_status_update.rb b/app/models/publisher_status_update.rb index 121799b70d..17ac0460bf 100644 --- a/app/models/publisher_status_update.rb +++ b/app/models/publisher_status_update.rb @@ -4,9 +4,10 @@ class PublisherStatusUpdate < ApplicationRecord ACTIVE = 'active'.freeze SUSPENDED = 'suspended'.freeze LOCKED = 'locked'.freeze + DELETED = 'deleted'.freeze NO_GRANTS = 'no_grants'.freeze - ALL_STATUSES = [CREATED, ONBOARDING, ACTIVE, SUSPENDED, LOCKED, NO_GRANTS].freeze + ALL_STATUSES = [CREATED, ONBOARDING, ACTIVE, SUSPENDED, LOCKED, NO_GRANTS, DELETED].freeze belongs_to :publisher diff --git a/app/views/admin/publishers/edit.html.slim b/app/views/admin/publishers/edit.html.slim index 0f06f1d149..3a9268bf0c 100644 --- a/app/views/admin/publishers/edit.html.slim +++ b/app/views/admin/publishers/edit.html.slim @@ -16,3 +16,5 @@ = " Uphold widget disabled" .form-group = f.submit class: "btn btn-primary" + = form_tag admin_publisher_path, method: :delete + = submit_tag "Delete account", class: 'btn btn-danger', data: { confirm: "Are you sure you want to delete this account?" } diff --git a/test/jobs/publisher_removal_job_test.rb b/test/jobs/publisher_removal_job_test.rb new file mode 100644 index 0000000000..eabee04f3e --- /dev/null +++ b/test/jobs/publisher_removal_job_test.rb @@ -0,0 +1,18 @@ +require 'test_helper' + +class PublisherRemovalJobTest < ActiveJob::TestCase + + test "deletes a publisher and his or her channels" do + publisher = publishers(:google_verified) + assert_not_equal 0, publisher.channels.count + PublisherRemovalJob.perform_now(publisher_id: publisher.id) + publisher.reload + assert_nil publisher.email + assert_nil publisher.pending_email + assert_equal PublisherStatusUpdate::DELETED, publisher.name + assert_nil publisher.last_sign_in_ip + assert_nil publisher.current_sign_in_ip + assert_equal 0, publisher.channels.count + assert_equal 0, publisher.versions.count + end +end diff --git a/test/models/publisher_status_update_test.rb b/test/models/publisher_status_update_test.rb index f38844b01c..c35e49dea1 100644 --- a/test/models/publisher_status_update_test.rb +++ b/test/models/publisher_status_update_test.rb @@ -20,6 +20,7 @@ class PublisherStatusUpdateTest < ActiveSupport::TestCase status_update = PublisherStatusUpdate.new(status: "created", publisher: publisher) status_update.save! + publisher.reload assert_equal publisher, status_update.publisher assert_equal publisher.status_updates.first, status_update end @@ -33,4 +34,4 @@ class PublisherStatusUpdateTest < ActiveSupport::TestCase status_update.save! end end -end \ No newline at end of file +end diff --git a/test/models/publisher_test.rb b/test/models/publisher_test.rb index c0009345a9..5bf491c933 100644 --- a/test/models/publisher_test.rb +++ b/test/models/publisher_test.rb @@ -224,43 +224,6 @@ class PublisherTest < ActiveSupport::TestCase end end - describe "#history" do - describe "when the publisher has notes" do - it 'shows just the notes' do - histories = publishers(:just_notes).history - histories.each do |history| - assert_equal history.class, PublisherNote - end - end - end - - describe "when the publisher has notes and statuses" do - it 'interweaves the objects' do - histories = publishers(:notes).history - - assert histories.any? { |h| h.is_a? PublisherNote } - assert histories.any? { |h| h.is_a? PublisherStatusUpdate } - end - - it 'sorts the object by created_at time' do - histories = publishers(:notes).history - histories.each_with_index do |history, index| - next if index == 0 - assert history.created_at < histories[index-1].created_at - end - end - end - - describe 'when the publisher just has statuses' do - it 'just shows the statuses' do - histories = publishers(:default).history - histories.each do |history| - assert_equal history.class, PublisherStatusUpdate - end - end - end - end - test "test `has_stale_uphold_code` scopes to correct publishers" do ActiveRecord::Base.record_timestamps = false publisher = publishers(:default) @@ -351,6 +314,7 @@ class PublisherTest < ActiveSupport::TestCase publisher.save! assert publisher.status_updates.count + publisher.reload assert publisher.last_status_update end @@ -529,4 +493,41 @@ class PublisherTest < ActiveSupport::TestCase publisher = publishers(:completed) assert publisher.most_recent_potential_referral_payment.blank? end + + describe "#history" do + describe "when the publisher has notes" do + it 'shows just the notes' do + histories = publishers(:just_notes).history + histories.each do |history| + assert_equal history.class, PublisherNote + end + end + end + + describe "when the publisher has notes and statuses" do + it 'interweaves the objects' do + histories = publishers(:notes).history + + assert histories.any? { |h| h.is_a? PublisherNote } + assert histories.any? { |h| h.is_a? PublisherStatusUpdate } + end + + it 'sorts the object by created_at time' do + histories = publishers(:notes).history + histories.each_with_index do |history, index| + next if index == 0 + assert history.created_at < histories[index-1].created_at + end + end + end + + describe 'when the publisher just has statuses' do + it 'just shows the statuses' do + histories = publishers(:default).history + histories.each do |history| + assert_equal history.class, PublisherStatusUpdate + end + end + end + end end