Add support for importing lists (#25203)
This commit is contained in:
		
					parent
					
						
							
								d91607feee
							
						
					
				
			
			
				commit
				
					
						8884d1ece0
					
				
			
		
					 9 changed files with 132 additions and 9 deletions
				
			
		|  | @ -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 | ||||
|  |  | |||
|  | @ -30,6 +30,7 @@ class BulkImport < ApplicationRecord | |||
|     muting: 2, | ||||
|     domain_blocking: 3, | ||||
|     bookmarks: 4, | ||||
|     lists: 5, | ||||
|   } | ||||
| 
 | ||||
|   enum state: { | ||||
|  |  | |||
|  | @ -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 | ||||
| 
 | ||||
|  |  | |||
|  | @ -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 | ||||
|  |  | |||
|  | @ -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 | ||||
|  |  | |||
|  | @ -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 | ||||
|  |  | |||
							
								
								
									
										3
									
								
								spec/fixtures/files/lists.csv
									
										
									
									
										vendored
									
									
										Normal file
									
								
							
							
						
						
									
										3
									
								
								spec/fixtures/files/lists.csv
									
										
									
									
										vendored
									
									
										Normal file
									
								
							|  | @ -0,0 +1,3 @@ | |||
| Mastodon project,gargron@example.com | ||||
| Mastodon project,mastodon@example.com | ||||
| test,foo@example.com | ||||
| 
 | 
|  | @ -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 | ||||
|  |  | |||
|  | @ -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 | ||||
|  |  | |||
		Loading…
	
	Add table
		Add a link
		
	
		Reference in a new issue