From 8884d1ece0eabd769e616f36785b6a7b14985469 Mon Sep 17 00:00:00 2001 From: Claire Date: Thu, 1 Jun 2023 14:47:31 +0200 Subject: [PATCH] Add support for importing lists (#25203) --- .../settings/imports_controller.rb | 4 ++ app/models/bulk_import.rb | 1 + app/models/form/import.rb | 19 +++-- app/services/bulk_import_row_service.rb | 8 ++- app/services/bulk_import_service.rb | 22 ++++++ app/views/settings/imports/index.html.haml | 2 +- spec/fixtures/files/lists.csv | 3 + spec/models/form/import_spec.rb | 10 +++ spec/services/bulk_import_row_service_spec.rb | 72 +++++++++++++++++++ 9 files changed, 132 insertions(+), 9 deletions(-) create mode 100644 spec/fixtures/files/lists.csv diff --git a/app/controllers/settings/imports_controller.rb b/app/controllers/settings/imports_controller.rb index bdbf8796fe..983caf22fa 100644 --- a/app/controllers/settings/imports_controller.rb +++ b/app/controllers/settings/imports_controller.rb @@ -12,6 +12,7 @@ class Settings::ImportsController < Settings::BaseController muting: 'muted_accounts_failures.csv', domain_blocking: 'blocked_domains_failures.csv', bookmarks: 'bookmarks_failures.csv', + lists: 'lists_failures.csv', }.freeze TYPE_TO_HEADERS_MAP = { @@ -20,6 +21,7 @@ class Settings::ImportsController < Settings::BaseController muting: ['Account address', 'Hide notifications'], domain_blocking: false, bookmarks: false, + lists: false, }.freeze def index @@ -49,6 +51,8 @@ class Settings::ImportsController < Settings::BaseController csv << [row.data['domain']] when :bookmarks csv << [row.data['uri']] + when :lists + csv << [row.data['list_name'], row.data['acct']] end end end diff --git a/app/models/bulk_import.rb b/app/models/bulk_import.rb index af9a9670bf..810e471849 100644 --- a/app/models/bulk_import.rb +++ b/app/models/bulk_import.rb @@ -30,6 +30,7 @@ class BulkImport < ApplicationRecord muting: 2, domain_blocking: 3, bookmarks: 4, + lists: 5, } enum state: { diff --git a/app/models/form/import.rb b/app/models/form/import.rb index 750ef84be4..2fc74715b5 100644 --- a/app/models/form/import.rb +++ b/app/models/form/import.rb @@ -18,6 +18,7 @@ class Form::Import muting: ['Account address', 'Hide notifications'], domain_blocking: ['#domain'], bookmarks: ['#uri'], + lists: ['List name', 'Account address'], }.freeze KNOWN_FIRST_HEADERS = EXPECTED_HEADERS_BY_TYPE.values.map(&:first).uniq.freeze @@ -30,6 +31,7 @@ class Form::Import 'Hide notifications' => 'hide_notifications', '#domain' => 'domain', '#uri' => 'uri', + 'List name' => 'list_name', }.freeze class EmptyFileError < StandardError; end @@ -48,6 +50,7 @@ class Form::Import return :muting if data.original_filename&.start_with?('mutes') || data.original_filename&.start_with?('muted_accounts') return :domain_blocking if data.original_filename&.start_with?('domain_blocks') || data.original_filename&.start_with?('blocked_domains') return :bookmarks if data.original_filename&.start_with?('bookmarks') + return :lists if data.original_filename&.start_with?('lists') end # Whether the uploaded CSV file seems to correspond to a different import type than the one selected @@ -76,14 +79,16 @@ class Form::Import private - def default_csv_header + def default_csv_headers case type.to_sym when :following, :blocking, :muting - 'Account address' + ['Account address'] when :domain_blocking - '#domain' + ['#domain'] when :bookmarks - '#uri' + ['#uri'] + when :lists + ['List name', 'Account address'] end end @@ -98,7 +103,7 @@ class Form::Import field&.split(',')&.map(&:strip)&.presence when 'Account address' field.strip.gsub(/\A@/, '') - when '#domain', '#uri' + when '#domain', '#uri', 'List name' field.strip else field @@ -109,7 +114,7 @@ class Form::Import @csv_data.take(1) # Ensure the headers are read raise EmptyFileError if @csv_data.headers == true - @csv_data = CSV.open(data.path, encoding: 'UTF-8', skip_blanks: true, headers: [default_csv_header], converters: csv_converter) unless KNOWN_FIRST_HEADERS.include?(@csv_data.headers&.first) + @csv_data = CSV.open(data.path, encoding: 'UTF-8', skip_blanks: true, headers: default_csv_headers, converters: csv_converter) unless KNOWN_FIRST_HEADERS.include?(@csv_data.headers&.first) @csv_data end @@ -133,7 +138,7 @@ class Form::Import def validate_data return if data.nil? return errors.add(:data, I18n.t('imports.errors.too_large')) if data.size > FILE_SIZE_LIMIT - return errors.add(:data, I18n.t('imports.errors.incompatible_type')) unless csv_data.headers.include?(default_csv_header) + return errors.add(:data, I18n.t('imports.errors.incompatible_type')) unless default_csv_headers.all? { |header| csv_data.headers.include?(header) } errors.add(:data, I18n.t('imports.errors.over_rows_processing_limit', count: ROWS_PROCESSING_LIMIT)) if csv_row_count > ROWS_PROCESSING_LIMIT diff --git a/app/services/bulk_import_row_service.rb b/app/services/bulk_import_row_service.rb index 4046ef4eed..ef4c18e789 100644 --- a/app/services/bulk_import_row_service.rb +++ b/app/services/bulk_import_row_service.rb @@ -7,7 +7,7 @@ class BulkImportRowService @type = row.bulk_import.type.to_sym case @type - when :following, :blocking, :muting + when :following, :blocking, :muting, :lists target_acct = @data['acct'] target_domain = domain(target_acct) @target_account = stoplight_wrap_request(target_domain) { ResolveAccountService.new.call(target_acct, { check_delivery_availability: true }) } @@ -33,6 +33,12 @@ class BulkImportRowService return false unless StatusPolicy.new(@account, @target_status).show? @account.bookmarks.find_or_create_by!(status: @target_status) + when :lists + list = @account.owned_lists.find_or_create_by!(title: @data['list_name']) + + FollowService.new.call(@account, @target_account) unless @account.id == @target_account.id + + list.accounts << @target_account end true diff --git a/app/services/bulk_import_service.rb b/app/services/bulk_import_service.rb index 2701b0c7e0..5c14adc499 100644 --- a/app/services/bulk_import_service.rb +++ b/app/services/bulk_import_service.rb @@ -16,6 +16,8 @@ class BulkImportService < BaseService import_domain_blocks! when :bookmarks import_bookmarks! + when :lists + import_lists! end @import.update!(state: :finished, finished_at: Time.now.utc) if @import.processed_items == @import.total_items @@ -157,4 +159,24 @@ class BulkImportService < BaseService [row.id] end end + + def import_lists! + rows = @import.rows.to_a + + if @import.overwrite? + included_lists = rows.map { |row| row.data['list_name'] }.uniq + + @account.owned_lists.where.not(title: included_lists).destroy_all + + # As list membership changes do not retroactively change timeline + # contents, simplify things by just clearing everything + @account.owned_lists.find_each do |list| + list.list_accounts.destroy_all + end + end + + Import::RowWorker.push_bulk(rows) do |row| + [row.id] + end + end end diff --git a/app/views/settings/imports/index.html.haml b/app/views/settings/imports/index.html.haml index 02c3f4eb3f..5f7950b598 100644 --- a/app/views/settings/imports/index.html.haml +++ b/app/views/settings/imports/index.html.haml @@ -3,7 +3,7 @@ = simple_form_for @import, url: settings_imports_path do |f| .field-group - = f.input :type, as: :grouped_select, collection: { constructive: %i(following bookmarks), destructive: %i(muting blocking domain_blocking) }, wrapper: :with_block_label, include_blank: false, label_method: ->(type) { I18n.t("imports.types.#{type}") }, group_label_method: ->(group) { I18n.t("imports.type_groups.#{group.first}") }, group_method: :last, hint: t('imports.preface') + = f.input :type, as: :grouped_select, collection: { constructive: %i(following bookmarks lists), destructive: %i(muting blocking domain_blocking) }, wrapper: :with_block_label, include_blank: false, label_method: ->(type) { I18n.t("imports.types.#{type}") }, group_label_method: ->(group) { I18n.t("imports.type_groups.#{group.first}") }, group_method: :last, hint: t('imports.preface') .fields-row .fields-group.fields-row__column.fields-row__column-6 diff --git a/spec/fixtures/files/lists.csv b/spec/fixtures/files/lists.csv new file mode 100644 index 0000000000..3155ed6d57 --- /dev/null +++ b/spec/fixtures/files/lists.csv @@ -0,0 +1,3 @@ +Mastodon project,gargron@example.com +Mastodon project,mastodon@example.com +test,foo@example.com diff --git a/spec/models/form/import_spec.rb b/spec/models/form/import_spec.rb index e1fea4205c..52cf1c96ed 100644 --- a/spec/models/form/import_spec.rb +++ b/spec/models/form/import_spec.rb @@ -86,6 +86,7 @@ RSpec.describe Form::Import do it_behaves_like 'too many CSV rows', 'muting', 'imports.txt', 1 it_behaves_like 'too many CSV rows', 'domain_blocking', 'domain_blocks.csv', 2 it_behaves_like 'too many CSV rows', 'bookmarks', 'bookmark-imports.txt', 3 + it_behaves_like 'too many CSV rows', 'lists', 'lists.csv', 2 # Importing list of addresses with no headers into various types it_behaves_like 'valid import', 'following', 'imports.txt' @@ -98,6 +99,9 @@ RSpec.describe Form::Import do # Importing bookmarks list with no headers into expected type it_behaves_like 'valid import', 'bookmarks', 'bookmark-imports.txt' + # Importing lists with no headers into expected type + it_behaves_like 'valid import', 'lists', 'lists.csv' + # Importing followed accounts with headers into various compatible types it_behaves_like 'valid import', 'following', 'following_accounts.csv' it_behaves_like 'valid import', 'blocking', 'following_accounts.csv' @@ -273,6 +277,12 @@ RSpec.describe Form::Import do { 'acct' => 'user@test.com', 'hide_notifications' => false }, ] + it_behaves_like 'on successful import', 'lists', 'merge', 'lists.csv', [ + { 'acct' => 'gargron@example.com', 'list_name' => 'Mastodon project' }, + { 'acct' => 'mastodon@example.com', 'list_name' => 'Mastodon project' }, + { 'acct' => 'foo@example.com', 'list_name' => 'test' }, + ] + # Based on the bug report 20571 where UTF-8 encoded domains were rejecting import of their users # # https://github.com/mastodon/mastodon/issues/20571 diff --git a/spec/services/bulk_import_row_service_spec.rb b/spec/services/bulk_import_row_service_spec.rb index 5bbe6b0042..5e09845b53 100644 --- a/spec/services/bulk_import_row_service_spec.rb +++ b/spec/services/bulk_import_row_service_spec.rb @@ -91,5 +91,77 @@ RSpec.describe BulkImportRowService do end end end + + context 'when importing a list row' do + let(:import_type) { 'lists' } + let(:target_account) { Fabricate(:account) } + let(:data) do + { 'acct' => target_account.acct, 'list_name' => 'my list' } + end + + shared_examples 'common behavior' do + context 'when the target account is already followed' do + before do + account.follow!(target_account) + end + + it 'returns true' do + expect(subject.call(import_row)).to be true + end + + it 'adds the target account to the list' do + expect { subject.call(import_row) }.to change { ListAccount.joins(:list).exists?(account_id: target_account.id, list: { title: 'my list' }) }.from(false).to(true) + end + end + + context 'when the user already requested to follow the target account' do + before do + account.request_follow!(target_account) + end + + it 'returns true' do + expect(subject.call(import_row)).to be true + end + + it 'adds the target account to the list' do + expect { subject.call(import_row) }.to change { ListAccount.joins(:list).exists?(account_id: target_account.id, list: { title: 'my list' }) }.from(false).to(true) + end + end + + context 'when the target account is neither followed nor requested' do + it 'returns true' do + expect(subject.call(import_row)).to be true + end + + it 'adds the target account to the list' do + expect { subject.call(import_row) }.to change { ListAccount.joins(:list).exists?(account_id: target_account.id, list: { title: 'my list' }) }.from(false).to(true) + end + end + + context 'when the target account is the user themself' do + let(:target_account) { account } + + it 'returns true' do + expect(subject.call(import_row)).to be true + end + + it 'adds the target account to the list' do + expect { subject.call(import_row) }.to change { ListAccount.joins(:list).exists?(account_id: target_account.id, list: { title: 'my list' }) }.from(false).to(true) + end + end + end + + context 'when the list does not exist yet' do + include_examples 'common behavior' + end + + context 'when the list exists' do + before do + Fabricate(:list, account: account, title: 'my list') + end + + include_examples 'common behavior' + end + end end end