From 765eca4c50e353d2491c6b5aa51bededb4f227e5 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 ++----- db/migrate/096_create_translation_tables.rb | 6 ++ ...20220902112339_remove_public_body_notes.rb | 13 ++++ spec/controllers/general_controller_spec.rb | 6 +- .../public_body_controller_spec.rb | 2 +- spec/factories/public_bodies.rb | 14 +++- spec/fixtures/note_translations.yml | 63 ++++++++++++++++++ spec/fixtures/notes.yml | 48 ++++++++++++++ spec/fixtures/public_bodies.yml | 3 +- spec/fixtures/public_body_translations.yml | 9 --- spec/lib/public_body_csv_spec.rb | 6 +- spec/models/public_body_spec.rb | 66 +++++-------------- spec/spec_helper.rb | 4 +- .../_search_result.html.erb_spec.rb | 33 ++++++---- 14 files changed, 194 insertions(+), 102 deletions(-) create mode 100644 db/migrate/20220902112339_remove_public_body_notes.rb create mode 100644 spec/fixtures/note_translations.yml create mode 100644 spec/fixtures/notes.yml diff --git a/app/models/public_body.rb b/app/models/public_body.rb index a8bd7c005d6..cd8f4d896e8 100644 --- a/app/models/public_body.rb +++ b/app/models/public_body.rb @@ -1,5 +1,5 @@ # == Schema Information -# Schema version: 20220210114052 +# Schema version: 20220928093559 # # 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/096_create_translation_tables.rb b/db/migrate/096_create_translation_tables.rb index a9402c51c13..a07e1cbf1d5 100644 --- a/db/migrate/096_create_translation_tables.rb +++ b/db/migrate/096_create_translation_tables.rb @@ -1,4 +1,10 @@ class CreateTranslationTables < ActiveRecord::Migration[4.2] # 2.3 + class ::PublicBody + # This has been removed from the model but is needed for this old migration + # to work + translates :notes + end + def self.up fields = { :name => :text, :short_name => :text, 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/controllers/public_body_controller_spec.rb b/spec/controllers/public_body_controller_spec.rb index 8dfcf0a1ce3..32d62c45e94 100644 --- a/spec/controllers/public_body_controller_spec.rb +++ b/spec/controllers/public_body_controller_spec.rb @@ -276,7 +276,7 @@ def make_single_language_example(locale) expect(assigns[:public_bodies]).to eq([ public_bodies(:humpadink_public_body) ]) end - it "should support simple searching of bodies by notes" do + xit "should support simple searching of bodies by notes" do get :list, params: { :public_body_query => 'Albatross' } expect(assigns[:public_bodies]).to eq([ public_bodies(:humpadink_public_body) ]) end diff --git a/spec/factories/public_bodies.rb b/spec/factories/public_bodies.rb index 53940d4c948..d509f7128ce 100644 --- a/spec/factories/public_bodies.rb +++ b/spec/factories/public_bodies.rb @@ -1,5 +1,5 @@ # == Schema Information -# Schema version: 20220210114052 +# Schema version: 20220928093559 # # 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 @@ -49,6 +48,17 @@ tag_string { 'eir_only' } end + trait :with_note do + transient do + note_body { 'This is my note' } + # note_locale { 'en' } + end + + concrete_notes do + [association(:note, body: note_body)] + end + end + factory :blank_email_public_body do request_email { '' } end diff --git a/spec/fixtures/note_translations.yml b/spec/fixtures/note_translations.yml new file mode 100644 index 00000000000..c55e7ff5987 --- /dev/null +++ b/spec/fixtures/note_translations.yml @@ -0,0 +1,63 @@ +geraldine_es_note_translation: + id: 1 + note_id: 2 + locale: es + body: + created_at: 2007-10-24 10:51:01.161639 + updated_at: 2007-10-24 10:51:01.161639 + +geraldine_en_note_translation: + id: 2 + note_id: 2 + locale: en + body: + created_at: 2007-10-24 10:51:01.161639 + updated_at: 2007-10-24 10:51:01.161639 + +humpadink_es_note_translation: + id: 3 + note_id: 3 + locale: es + body: Baguette + created_at: 2007-10-24 10:51:01.161639 + updated_at: 2007-10-24 10:51:01.161639 + +humpadink_en_note_translation: + id: 4 + note_id: 3 + locale: en + body: An albatross told me!!! + created_at: 2007-10-24 10:51:01.161639 + updated_at: 2007-10-24 10:51:01.161639 + +silly_walks_en_note_translation: + id: 6 + note_id: 5 + locale: en + body: You know the one. + created_at: 2007-10-24 10:51:01.161639 + updated_at: 2007-10-24 10:51:01.161639 + +sensible_walks_en_note_translation: + id: 7 + note_id: 6 + locale: en + body: I bet you’ve never heard of it. + created_at: 2008-10-25 10:51:01.161639 + updated_at: 2008-10-25 10:51:01.161639 + +other_note_translation: + id: 8 + note_id: 7 + locale: en + body: More notes + created_at: 2008-10-25 10:51:01.161639 + updated_at: 2008-10-25 10:51:01.161639 + +humpadink_he_IL_note_translation: + id: 9 + note_id: 3 + locale: he_IL + body: An albatross told me!!! + created_at: 2007-10-24 10:51:01.161639 + updated_at: 2007-10-24 10:51:01.161639 diff --git a/spec/fixtures/notes.yml b/spec/fixtures/notes.yml new file mode 100644 index 00000000000..27657191414 --- /dev/null +++ b/spec/fixtures/notes.yml @@ -0,0 +1,48 @@ +# == Schema Information +# Schema version: 20220928093559 +# +# Table name: notes +# +# id :bigint not null, primary key +# notable_type :string +# notable_id :bigint +# notable_tag :string +# created_at :datetime not null +# updated_at :datetime not null +# body :text +# + +geraldine_note: + id: 2 + notable_type: PublicBody + notable_id: 2 + created_at: 2007-10-24 10:51:01.161639 + updated_at: 2007-10-24 10:51:01.161639 + +humpadink_note: + id: 3 + notable_type: PublicBody + notable_id: 3 + created_at: 2007-10-24 10:51:01.161639 + updated_at: 2007-10-24 10:51:01.161639 + +silly_walks_note: + id: 5 + notable_type: PublicBody + notable_id: 5 + created_at: 2007-10-24 10:51:01.161639 + updated_at: 2007-10-24 10:51:01.161639 + +sensible_walks_note: + id: 6 + notable_type: PublicBody + notable_id: 6 + created_at: 2008-10-25 10:51:01.161639 + updated_at: 2008-10-25 10:51:01.161639 + +other_note: + id: 7 + notable_type: PublicBody + notable_id: 7 + created_at: 2008-10-25 10:51:01.161639 + updated_at: 2008-10-25 10:51:01.161639 diff --git a/spec/fixtures/public_bodies.yml b/spec/fixtures/public_bodies.yml index bd7644915ea..25a2752d056 100644 --- a/spec/fixtures/public_bodies.yml +++ b/spec/fixtures/public_bodies.yml @@ -1,5 +1,5 @@ # == Schema Information -# Schema version: 20220210114052 +# Schema version: 20220928093559 # # 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..d983128e7ae 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.concrete_notes.create(body: 'An exported authority') csv = PublicBodyCSV.new csv << body @@ -138,19 +138,19 @@ :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.concrete_notes.create(body: 'An exported authority') attrs2 = { :name => 'Exported to CSV 2', :short_name => 'CSV2', :request_email => 'csv2@localhost', :tag_string => 'exported', - :notes => 'Exported authority', :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.concrete_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..d6eb7443685 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: 20220928093559 # # 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 @@ -545,7 +544,7 @@ subject(:notes) { public_body.notes } 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 @@ -558,13 +557,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 +570,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 +583,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 +1204,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. diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 2e0a2d9bb44..0b426f0d8d2 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -63,7 +63,9 @@ :public_body_category_translations, :public_body_headings, :public_body_heading_translations, - :public_body_category_links + :public_body_category_links, + :notes, + :note_translations # If you're not using ActiveRecord, or you'd prefer not to run each of your # examples within a transaction, remove the following line or assign false diff --git a/spec/views/alaveteli_pro/public_bodies/_search_result.html.erb_spec.rb b/spec/views/alaveteli_pro/public_bodies/_search_result.html.erb_spec.rb index 8fa17817d00..b3f587455d6 100644 --- a/spec/views/alaveteli_pro/public_bodies/_search_result.html.erb_spec.rb +++ b/spec/views/alaveteli_pro/public_bodies/_search_result.html.erb_spec.rb @@ -1,9 +1,12 @@ require 'spec_helper' RSpec.describe 'alaveteli_pro/public_bodies/_search_result' do + let(:note) { 'Some notes about the body' } + let(:public_body) do - FactoryBot.create(:public_body, notes: "Some notes about the body", - info_requests_visible_count: 1) + FactoryBot.create(:public_body, :with_note, + note_body: note, + info_requests_visible_count: 1) end let(:result) do @@ -28,17 +31,21 @@ def render_view expect(rendered).to have_text public_body.notes_as_string end - it "truncates the body notes to 150 chars" do - public_body.notes = "This are some extravagantly long notes about a " \ - "body which will need to be trimmed down somewhat " \ - "before they're suitable for inclusion in a small " \ - "amount of space." - render_view - expected_notes = "This are some extravagantly long notes about a body " \ - "which will need to be trimmed down somewhat before " \ - "they're suitable for inclusion in a small am..." - expect(rendered).not_to have_text public_body.notes_as_string - expect(rendered).to have_text expected_notes + context 'long note' do + let(:note) do + "This are some extravagantly long notes about a body which will need " \ + "to be trimmed down somewhat before they're suitable for inclusion in " \ + "a small amount of space." + end + + it "truncates the body notes to 150 chars" do + render_view + expected_notes = "This are some extravagantly long notes about a body " \ + "which will need to be trimmed down somewhat before " \ + "they're suitable for inclusion in a small am..." + expect(rendered).not_to have_text public_body.notes_as_string + expect(rendered).to have_text expected_notes + end end it "includes the number of requests made" do