From f6b6a0d22810c1041a1d6749a8e8f66be3c26415 Mon Sep 17 00:00:00 2001 From: syed-ali-tw Date: Thu, 27 Feb 2025 13:17:15 +0000 Subject: [PATCH 1/2] Move Artefact and ArtefactAction to postgres --- app/controllers/downtimes_controller.rb | 2 +- app/models/artefact.rb | 86 +++--------------- app/models/artefact_action.rb | 32 +------ app/models/artefact_external_link.rb | 9 +- app/models/downtime.rb | 18 +++- app/models/user.rb | 2 + .../shared/_legacy_error_summary.html.erb | 2 +- .../20250227112626_create_artefact_action.rb | 12 +++ db/migrate/20250227113655_create_artefact.rb | 25 ++++++ ...228161852_create_artefact_external_link.rb | 10 +++ db/schema.rb | 49 ++++++++++- lib/csv_report_generator.rb | 4 +- test/functional/downtimes_controller_test.rb | 2 +- test/functional/editions_controller_test.rb | 6 +- test/integration/downtime_integration_test.rb | 2 +- .../downtime_with_invalid_dates_test.rb | 2 +- test/integration/edition_edit_test.rb | 4 +- .../edition_external_links_test.rb | 2 +- test/integration/tagging_test.rb | 4 +- test/models/artefact_action_test.rb | 51 ++++++----- test/models/artefact_external_link_test.rb | 16 ++-- test/models/artefact_test.rb | 2 +- test/models/downtime_test.rb | 2 +- test/models/time_zone_test.rb | 88 ++++++++++--------- test/support/factories.rb | 2 +- test/unit/all_urls_presenter_test.rb | 4 +- test/unit/helpers/downtimes_helper_test.rb | 2 +- .../organisation_content_presenter_test.rb | 4 +- .../formats/answer_presenter_test.rb | 6 +- .../completed_transaction_presenter_test.rb | 6 +- .../formats/generic_edition_presenter_test.rb | 3 +- .../formats/guide_presenter_test.rb | 6 +- .../formats/help_page_presenter_test.rb | 6 +- .../formats/licence_presenter_test.rb | 6 +- .../local_transaction_presenter_test.rb | 6 +- .../formats/place_presenter_test.rb | 7 +- .../simple_smart_answer_presenter_test.rb | 6 +- .../formats/transaction_presenter_test.rb | 8 +- 38 files changed, 269 insertions(+), 235 deletions(-) create mode 100644 db/migrate/20250227112626_create_artefact_action.rb create mode 100644 db/migrate/20250227113655_create_artefact.rb create mode 100644 db/migrate/20250228161852_create_artefact_external_link.rb diff --git a/app/controllers/downtimes_controller.rb b/app/controllers/downtimes_controller.rb index 1da37aa06..861d2ed07 100644 --- a/app/controllers/downtimes_controller.rb +++ b/app/controllers/downtimes_controller.rb @@ -9,7 +9,7 @@ def index end def new - @downtime = Downtime.new(artefact: @edition.artefact) + @downtime = Downtime.new(artefact_id: @edition.artefact.id) end def create diff --git a/app/models/artefact.rb b/app/models/artefact.rb index 063598c9d..d32a24557 100644 --- a/app/models/artefact.rb +++ b/app/models/artefact.rb @@ -2,45 +2,9 @@ require "artefact_action" # Require this when running outside Rails require_dependency "safe_html" -class Artefact - include Mongoid::Document - include Mongoid::Timestamps - +class Artefact < ApplicationRecord strip_attributes only: :redirect_url - field "name", type: String - field "slug", type: String - field "paths", type: Array, default: [] - field "prefixes", type: Array, default: [] - field "kind", type: String - field "owning_app", type: String - field "rendering_app", type: String - field "active", type: Boolean, default: false - - field "publication_id", type: String - field "description", type: String - field "state", type: String, default: "draft" - field "language", type: String, default: "en" - field "latest_change_note", type: String - field "public_timestamp", type: DateTime - field "redirect_url", type: String - - # content_id should be unique but we have existing artefacts without it. - # We should therefore enforce the uniqueness as soon as: - # - every current artefact will have a content id assigned - # - every future artefact will be created with a content id - field "content_id", type: String - - index({ slug: 1 }, unique: true) - - # This index allows the `relatable_artefacts` method to use an index-covered - # query, so it doesn't have to load each of the artefacts. - index name: 1, - state: 1, - kind: 1, - _type: 1, - _id: 1 - scope :not_archived, -> { where(:state.nin => %w[archived]) } FORMATS_BY_DEFAULT_OWNING_APP = { @@ -82,9 +46,9 @@ def self.default_app_for_format(format) "find my nearest" => "place", }.tap { |h| h.default_proc = ->(_, k) { k } }.freeze - embeds_many :actions, class_name: "ArtefactAction", order: { created_at: :asc } + has_many :artefact_actions, -> { order(created_at: :asc) }, class_name: "ArtefactAction", dependent: :destroy - embeds_many :external_links, class_name: "ArtefactExternalLink" + has_many :external_links, class_name: "ArtefactExternalLink" accepts_nested_attributes_for :external_links, allow_destroy: true, reject_if: proc { |attrs| attrs["title"].blank? && attrs["url"].blank? } @@ -92,7 +56,6 @@ def self.default_app_for_format(format) before_validation :normalise, on: :create before_create :record_create_action before_update :record_update_action - before_update :remember_if_slug_has_changed after_update :update_editions validates :name, presence: { message: "Enter a title" } @@ -146,7 +109,7 @@ def any_editions_ever_published? def update_editions return archive_editions if state == "archived" - if @slug_was_changed + if saved_change_to_attribute?("slug") Edition.draft_in_publishing_api.where(panopticon_id: id).each do |edition| edition.update_slug_from_artefact(self) end @@ -171,15 +134,14 @@ def update_as(user, *args) save_as user end - # Return value is used in caller chain to show errors - # rubocop:disable Rails/SaveBang + # 'valid?' populates the error context for the instance which is used in caller chain to show errors def save_as(user, options = {}) default_action = new_record? ? "create" : "update" action_type = options.delete(:action_type) || default_action record_action(action_type, user:) - save(options) + + save! if valid? end - # rubocop:enable Rails/SaveBang # We should use this method when performing save actions from rake tasks, # message queue consumer or any other performed tasks that have no user associated @@ -187,9 +149,9 @@ def save_as(user, options = {}) def save_as_task!(task_name, options = {}) default_action = new_record? ? "create" : "update" action_type = options.delete(:action_type) || default_action - record_action(action_type, task_name:) - save!(options) + + save! if valid? end def record_create_action @@ -204,7 +166,7 @@ def record_action(action_type, options = {}) user = options[:user] task_name = options[:task_name] current_snapshot = snapshot - last_snapshot = actions.last.snapshot if actions.last + last_snapshot = artefact_actions.last.snapshot if artefact_actions.last unless current_snapshot == last_snapshot @@ -216,25 +178,7 @@ def record_action(action_type, options = {}) attributes[:user] = user if user attributes[:task_performed_by] = task_name if task_name - # Temp-to-be-removed - # This will be removed once we move artefact table to postgres, currently record_action - # when called with options contains the user object attributes that is supported by belongs to - # the below code allows to use the user id foreign key instead as we are temporarily not using belongs to - # relationship between artefact and user - add_user_id_and_delete_user_key(attributes) - new_action = actions.build(attributes) - # Mongoid will not fire creation callbacks on embedded documents, so we - # need to trigger this manually. There is a `cascade_callbacks` option on - # `embeds_many`, but it doesn't appear to trigger creation events on - # children when an update event fires on the parent - new_action.set_created_at - end - end - - def add_user_id_and_delete_user_key(attributes) - if attributes[:user] - attributes[:user_id] = attributes[:user].id - attributes.delete(:user) + artefact_actions.build(attributes) end end @@ -248,7 +192,7 @@ def live? def snapshot attributes - .except("_id", "created_at", "updated_at", "actions") + .except("id", "created_at", "updated_at", "artefact_actions") end def latest_edition @@ -285,12 +229,6 @@ def exact_route? private - # We need to do this because Mongoid doesn't implement 'saved_change_to_attribute?' methods like ActiveRecord does - # https://api.rubyonrails.org/classes/ActiveRecord/AttributeMethods/Dirty.html#method-i-saved_change_to_attribute-3F - def remember_if_slug_has_changed - @slug_was_changed = slug_changed? - end - def edition_class_name "#{kind.camelcase}Edition" end diff --git a/app/models/artefact_action.rb b/app/models/artefact_action.rb index 533bce5fd..2dd582582 100644 --- a/app/models/artefact_action.rb +++ b/app/models/artefact_action.rb @@ -1,32 +1,6 @@ -class ArtefactAction - include Mongoid::Document - include Mongoid::Timestamps::Created - - field "action_type", type: String - field "snapshot", type: Hash - field "task_performed_by", type: String - - # Temp-to-be-removed - # This will be removed once we move artefact_action table to postgres, this temporarily - # allows to support the belongs_to relation between artefact_action and user - field "user_id", type: BSON::ObjectId - - embedded_in :artefact - - # Ideally we would like to use the UID field here, since that will be the - # same across all applications, but Mongoid doesn't yet support using a - # custom primary key on a related field - - # Temp-to-be-brought-back - # Currently we are using user_id as a field to store the user_id - # to bypass the issue of having a belongs_to between a postgres table and a mongo table - # we will most likely bring back the belongs_to relationship once we move artefact_action table to postgres. - - # belongs_to :user, optional: true - - def user - User.find(user_id) if user_id - end +class ArtefactAction < ApplicationRecord + belongs_to :artefact + belongs_to :user, optional: true # Not validating presence of a user just yet, since there may be some # circumstances where we can't reliably determine the user. As an example diff --git a/app/models/artefact_external_link.rb b/app/models/artefact_external_link.rb index 4c7836976..2931379a9 100644 --- a/app/models/artefact_external_link.rb +++ b/app/models/artefact_external_link.rb @@ -1,12 +1,7 @@ -class ArtefactExternalLink - include Mongoid::Document - +class ArtefactExternalLink < ApplicationRecord strip_attributes only: :url - field "title", type: String - field "url", type: String - - embedded_in :artefact + belongs_to :artefact validates :title, presence: true validates :url, presence: true, format: { with: URI::DEFAULT_PARSER.make_regexp(%w[http https]) } diff --git a/app/models/downtime.rb b/app/models/downtime.rb index 54ab6c736..886c13302 100644 --- a/app/models/downtime.rb +++ b/app/models/downtime.rb @@ -5,8 +5,13 @@ class Downtime field :message, type: String field :start_time, type: DateTime field :end_time, type: DateTime + field :artefact_id, type: Integer - belongs_to :artefact, optional: true + # Temp-to-be-brought-back + # Currently we are using artefact_id as a field to store the artefact id + # to bypass the issue of having a belongs_to between a postgres table and a mongo table + # we will most likely bring back the belongs_to relationship once we move Downtime table to postgres. + # belongs_to :artefact, optional: true validate :start_time_precedes_end_time validate :end_time_is_in_future @@ -24,6 +29,17 @@ def display_start_time start_time.yesterday.at_midnight end + # Temp-to-be-removed + # This will be removed once we move Downtime table to postgres, this temporarily + # allows to support the belongs_to relation between Downtime and Artefact + def artefact_id=(id) + self[:artefact_id] = id + end + + def artefact + Artefact.find(artefact_id) if artefact_id + end + private def end_time_is_in_future diff --git a/app/models/user.rb b/app/models/user.rb index 2e82b1fac..5547bc41d 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -6,6 +6,8 @@ class User < ApplicationRecord include GDS::SSO::User + has_many :artefact_actions, class_name: "ArtefactAction" + scope :alphabetized, -> { order(name: :asc) } scope :enabled, -> { where("disabled IS NULL OR disabled = ?", false) } diff --git a/app/views/shared/_legacy_error_summary.html.erb b/app/views/shared/_legacy_error_summary.html.erb index 71621c067..5da498881 100644 --- a/app/views/shared/_legacy_error_summary.html.erb +++ b/app/views/shared/_legacy_error_summary.html.erb @@ -4,7 +4,7 @@