From 359b4c669475d22b8c53cada77729f5090adc24c Mon Sep 17 00:00:00 2001 From: Graeme Porteous Date: Fri, 2 Sep 2022 12:28:50 +0100 Subject: [PATCH] Remove PublicBody#notes This has been replace by a separate Note model. Fixes #7254 --- app/models/public_body.rb | 23 ++----- ...20220902112339_remove_public_body_notes.rb | 13 ++++ spec/controllers/general_controller_spec.rb | 6 +- spec/factories/public_bodies.rb | 3 +- spec/fixtures/public_bodies.yml | 3 +- spec/fixtures/public_body_translations.yml | 9 --- spec/lib/public_body_csv_spec.rb | 5 +- spec/models/public_body_spec.rb | 68 +++++-------------- 8 files changed, 42 insertions(+), 88 deletions(-) create mode 100644 db/migrate/20220902112339_remove_public_body_notes.rb diff --git a/app/models/public_body.rb b/app/models/public_body.rb index a8bd7c005d6..63ba2d32c38 100644 --- a/app/models/public_body.rb +++ b/app/models/public_body.rb @@ -1,5 +1,5 @@ # == Schema Information -# Schema version: 20220210114052 +# Schema version: 20220902112339 # # Table name: public_bodies # @@ -22,7 +22,6 @@ # short_name :text # request_email :text # url_name :text -# notes :text # first_letter :string # publication_scheme :text # disclosure_log :text @@ -136,7 +135,7 @@ def self.admin_title strip_attributes allow_empty: false, except: %i[request_email] strip_attributes allow_empty: true, only: %i[request_email] - translates :name, :short_name, :request_email, :url_name, :notes, :first_letter, :publication_scheme + translates :name, :short_name, :request_email, :url_name, :first_letter, :publication_scheme # Cannot be grouped at top as it depends on the `translates` macro include Translatable @@ -439,7 +438,6 @@ def self.internal_admin_body :short_name => "", :request_email => AlaveteliConfiguration.contact_email, :home_page => nil, - :notes => nil, :publication_scheme => nil, :last_edit_editor => "internal_admin", :last_edit_comment => @@ -669,25 +667,13 @@ def self.extract_domain_from_email(email) end def notes - [legacy_note].compact + all_notes + all_notes end def notes_as_string notes.map(&:body).join(' ') end - def legacy_note - return unless read_attribute(:notes).present? - - Note.new(notable: self) do |note| - AlaveteliLocalization.available_locales.each do |locale| - AlaveteliLocalization.with_locale(locale) do - note.body = read_attribute(:notes) - end - end - end - end - def has_notes? notes.present? end @@ -966,8 +952,7 @@ def name_for_search def self.get_public_body_list_translated_condition(table, has_first_letter=false, locale=nil) result = "(upper(#{table}.name) LIKE upper(:query)" \ - " OR upper(#{table}.notes) LIKE upper(:query)" \ - " OR upper(#{table}.short_name) LIKE upper(:query))" + " OR upper(#{table}.short_name) LIKE upper(:query))" if has_first_letter result += " AND #{table}.first_letter = :first_letter" end diff --git a/db/migrate/20220902112339_remove_public_body_notes.rb b/db/migrate/20220902112339_remove_public_body_notes.rb new file mode 100644 index 00000000000..90491f35c05 --- /dev/null +++ b/db/migrate/20220902112339_remove_public_body_notes.rb @@ -0,0 +1,13 @@ +class RemovePublicBodyNotes < ActiveRecord::Migration[6.1] + def change + reversible do |dir| + dir.up do + remove_column :public_body_translations, :notes + end + + dir.down do + PublicBody.add_translation_fields! notes: :text + end + end + end +end diff --git a/spec/controllers/general_controller_spec.rb b/spec/controllers/general_controller_spec.rb index 5f6246ac4a2..d4b095ee1b7 100644 --- a/spec/controllers/general_controller_spec.rb +++ b/spec/controllers/general_controller_spec.rb @@ -305,13 +305,11 @@ it 'should prioritise direct matches of public body names' do FactoryBot. create(:public_body, - name: 'Cardiff Business Technology Centre Limited', - notes: 'Something something cardiff council something else.') + name: 'Cardiff Business Technology Centre Limited') FactoryBot. create(:public_body, - name: 'Cardiff and Vale of Glamorgan Community Health Council', - notes: 'Another notes mentioning Cardiff Council.') + name: 'Cardiff and Vale of Glamorgan Community Health Council') FactoryBot.create(:public_body, name: 'Cardiff Council') diff --git a/spec/factories/public_bodies.rb b/spec/factories/public_bodies.rb index 53940d4c948..89a32851558 100644 --- a/spec/factories/public_bodies.rb +++ b/spec/factories/public_bodies.rb @@ -1,5 +1,5 @@ # == Schema Information -# Schema version: 20220210114052 +# Schema version: 20220902112339 # # Table name: public_bodies # @@ -22,7 +22,6 @@ # short_name :text # request_email :text # url_name :text -# notes :text # first_letter :string # publication_scheme :text # disclosure_log :text diff --git a/spec/fixtures/public_bodies.yml b/spec/fixtures/public_bodies.yml index bd7644915ea..9c95ea4760e 100644 --- a/spec/fixtures/public_bodies.yml +++ b/spec/fixtures/public_bodies.yml @@ -1,5 +1,5 @@ # == Schema Information -# Schema version: 20220210114052 +# Schema version: 20220902112339 # # Table name: public_bodies # @@ -22,7 +22,6 @@ # short_name :text # request_email :text # url_name :text -# notes :text # first_letter :string # publication_scheme :text # disclosure_log :text diff --git a/spec/fixtures/public_body_translations.yml b/spec/fixtures/public_body_translations.yml index 8d645912ad0..3b13fcaf72c 100644 --- a/spec/fixtures/public_body_translations.yml +++ b/spec/fixtures/public_body_translations.yml @@ -7,7 +7,6 @@ geraldine_es_public_body_translation: short_name: eTGQ url_name: etgq locale: es - notes: "" publication_scheme: "" disclosure_log: "" created_at: 2007-10-24 10:51:01.161639 @@ -22,7 +21,6 @@ geraldine_en_public_body_translation: short_name: TGQ url_name: tgq locale: en - notes: "" publication_scheme: "" disclosure_log: "" created_at: 2007-10-24 10:51:01.161639 @@ -37,7 +35,6 @@ humpadink_es_public_body_translation: short_name: eDfH url_name: edfh locale: es - notes: Baguette publication_scheme: "" disclosure_log: "" created_at: 2007-10-24 10:51:01.161639 @@ -52,7 +49,6 @@ humpadink_en_public_body_translation: short_name: DfH url_name: dfh locale: en - notes: An albatross told me!!! publication_scheme: "" disclosure_log: "" created_at: 2007-10-24 10:51:01.161639 @@ -67,7 +63,6 @@ forlorn_en_public_body_translation: short_name: DoL url_name: lonely locale: en - notes: A very lonely public body that no one has corresponded with publication_scheme: "" disclosure_log: "" created_at: 2007-10-24 10:51:01.161639 @@ -82,7 +77,6 @@ silly_walks_en_public_body_translation: request_email: silly-walks-requests@localhost short_name: MSW url_name: msw - notes: You know the one. publication_scheme: "" disclosure_log: "" created_at: 2007-10-24 10:51:01.161639 @@ -97,7 +91,6 @@ sensible_walks_en_public_body_translation: request_email: sensible-walks-requests@localhost short_name: SenseWalk url_name: sensible_walks - notes: I bet you’ve never heard of it. publication_scheme: "" disclosure_log: "" created_at: 2008-10-25 10:51:01.161639 @@ -112,7 +105,6 @@ other_public_body_translation: request_email: other@localhost short_name: Another Public Body url_name: another_public_body - notes: More notes publication_scheme: "" disclosure_log: "" created_at: 2008-10-25 10:51:01.161639 @@ -127,7 +119,6 @@ humpadink_he_IL_public_body_translation: short_name: DfH url_name: dfh locale: he_IL - notes: An albatross told me!!! publication_scheme: "" disclosure_log: "" created_at: 2007-10-24 10:51:01.161639 diff --git a/spec/lib/public_body_csv_spec.rb b/spec/lib/public_body_csv_spec.rb index d070762d4f9..a08a8169ffb 100644 --- a/spec/lib/public_body_csv_spec.rb +++ b/spec/lib/public_body_csv_spec.rb @@ -117,10 +117,10 @@ :short_name => 'CSV', :request_email => 'csv@localhost', :tag_string => 'exported', - :notes => 'An exported authority', :created_at => '2007-10-25 10:51:01 UTC', :updated_at => '2007-10-25 10:51:01 UTC' } body = FactoryBot.create(:public_body, attrs) + body.notes.create(body: 'An exported authority') csv = PublicBodyCSV.new csv << body @@ -138,10 +138,10 @@ :short_name => 'CSV1', :request_email => 'csv1@localhost', :tag_string => 'exported', - :notes => 'An exported authority', :created_at => '2007-10-25 10:51:01 UTC', :updated_at => '2007-10-25 10:51:01 UTC' } body1 = FactoryBot.create(:public_body, attrs1) + body1.notes.create(body: 'An exported authority') attrs2 = { :name => 'Exported to CSV 2', :short_name => 'CSV2', @@ -151,6 +151,7 @@ :created_at => '2011-01-26 14:11:02 UTC', :updated_at => '2011-01-26 14:11:02 UTC' } body2 = FactoryBot.create(:public_body, attrs2) + body2.notes.create(body: 'Exported authority') expected = <<-CSV.strip_heredoc Name,Short name,URL name,Home page,Publication scheme,Disclosure log,Notes,Created at,Updated at,Version diff --git a/spec/models/public_body_spec.rb b/spec/models/public_body_spec.rb index e27207fc760..33e580ea515 100644 --- a/spec/models/public_body_spec.rb +++ b/spec/models/public_body_spec.rb @@ -1,5 +1,5 @@ # == Schema Information -# Schema version: 20220210114052 +# Schema version: 20220902112339 # # Table name: public_bodies # @@ -22,7 +22,6 @@ # short_name :text # request_email :text # url_name :text -# notes :text # first_letter :string # publication_scheme :text # disclosure_log :text @@ -544,9 +543,7 @@ describe '#notes' do subject(:notes) { public_body.notes } - let(:public_body) do - FactoryBot.build(:public_body, notes: 'foo', tag_string: 'important') - end + let(:public_body) { FactoryBot.build(:public_body) } let!(:concrete_note) do FactoryBot.create(:note, :for_public_body, notable: public_body) @@ -558,13 +555,12 @@ it 'returns an array' do is_expected.to be_an Array - expect(notes.count).to eq 3 + expect(notes.count).to eq 2 end it 'combined notable notes with legacy note' do - expect(notes[0].body).to eq 'foo' - expect(notes[1]).to eq concrete_note - expect(notes[2]).to eq tagged_note + expect(notes[0]).to eq concrete_note + expect(notes[1]).to eq tagged_note end end @@ -572,7 +568,7 @@ subject(:notes) { public_body.notes_as_string } let(:public_body) do - FactoryBot.build(:public_body, notes: 'foo', tag_string: 'important') + FactoryBot.build(:public_body, tag_string: 'important') end let!(:concrete_note) do @@ -585,56 +581,28 @@ end it 'concaterates note bodies' do - is_expected.to eq('foo bar baz') - end - end - - describe '#legacy_note' do - subject(:legacy_note) { public_body.legacy_note } - - context 'without legacy translated attributes' do - let(:public_body) { FactoryBot.build(:public_body) } - it { is_expected.to be_nil } - end - - context 'with legacy translated attributes' do - let(:public_body) do - FactoryBot.build( - :public_body, - notes: 'foo', - translations_attributes: { es: { locale: 'es', notes: 'bar' } } - ) - end - - it 'builds new note instance' do - is_expected.to be_a Note - expect(legacy_note.body).to eq 'foo' - AlaveteliLocalization.with_locale('es') do - expect(legacy_note.body).to eq 'bar' - end - end - - it 'assigns body as notable' do - expect(legacy_note.notable).to eq public_body - end + is_expected.to eq('bar baz') end end describe '#has_notes?' do + subject { public_body.has_notes? } + let(:public_body) { PublicBody.new } + it 'returns false if notes is nil' do - subject = PublicBody.new(:notes => nil) - expect(subject.has_notes?).to eq(false) + allow(public_body).to receive(:notes).and_return(nil) + is_expected.to eq(false) end - it 'returns false if notes is blank' do - subject = PublicBody.new(:notes => '') - expect(subject.has_notes?).to eq(false) + it 'returns false if notes is empty' do + allow(public_body).to receive(:notes).and_return([]) + is_expected.to eq(false) end it 'returns true if notes are present' do - subject = PublicBody.new(:notes => 'x') - expect(subject.has_notes?).to eq(true) + allow(public_body).to receive(:notes).and_return([double(:note)]) + is_expected.to eq(true) end end @@ -1234,7 +1202,7 @@ def set_default_attributes(public_body) ActsAsXapian::ActsAsXapianJob.destroy_all - body.update!(notes: 'test') + body.update!(request_email: 'other@localhost') expected_events = ActsAsXapian::ActsAsXapianJob.