Merge pull request #2564 from alphagov/p-o-p-b-migrate-artefact-artef…

…actaction

Move artefact and artefact_action table to postgres

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

app/models/action.rb

@@ -51,8 +51,8 @@ class Action
   # Temp-to-be-removed
   # This will be removed once we move action table to postgres, this temporarily
   # allows to support the belongs to relation between action and user
-  field :recipient_id,       type: BSON::ObjectId
-  field :requester_id,       type: BSON::ObjectId
+  field :recipient_id,       type: Integer
+  field :requester_id,       type: Integer
 
   def container_class_name(edition)
     edition.container.class.name.underscore.humanize

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,17 +46,16 @@ 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? }
 
   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,25 +134,24 @@ 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
   # as we are still interested to know what triggered the action.
   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

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

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]) }

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

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) }
 

app/views/shared/_legacy_error_summary.html.erb

@@ -4,7 +4,7 @@
     <ul>
       <% object.errors.each do |error| %>
       <li>
-        <% model = object.class.superclass.to_s == "Object" ? object.class : object.class.superclass %>
+        <% model = object.class.superclass.to_s == "ApplicationRecord" ? object.class : object.class.superclass %>
         <a href="#<%= model.to_s.underscore + "_" + error.attribute.to_s %>"><%= error.message %></a>
       </li>
       <% end %>

db/migrate/20250227112626_create_artefact_action.rb

@@ -0,0 +1,12 @@
+class CreateArtefactAction < ActiveRecord::Migration[7.1]
+  def change
+    create_table :artefact_actions do |t|
+      t.string :action_type
+      t.jsonb :snapshot
+      t.string :task_performed_by
+      t.timestamps
+      t.references :user, foreign_key: true
+      t.references :artefact, index: true, foreign_key: true
+    end
+  end
+end

db/migrate/20250227113655_create_artefact.rb

@@ -0,0 +1,25 @@
+class CreateArtefact < ActiveRecord::Migration[7.1]
+  def change
+    create_table :artefacts do |t|
+      t.string :name
+      t.string :slug
+      t.string :paths, array: true, default: []
+      t.string :prefixes, array: true, default: []
+      t.string :kind
+      t.string :owning_app
+      t.string :rendering_app
+      t.boolean :active, default: false
+      t.string :publication_id
+      t.string :description
+      t.string :state, default: "draft"
+      t.string :language, default: "en"
+      t.string :latest_change_note
+      t.datetime :public_timestamp
+      t.string :redirect_url
+      t.string :content_id
+      t.timestamps
+      t.index %i[name state kind id]
+      t.index :slug, unique: true
+    end
+  end
+end

db/migrate/20250228161852_create_artefact_external_link.rb

@@ -0,0 +1,10 @@
+class CreateArtefactExternalLink < ActiveRecord::Migration[7.1]
+  def change
+    create_table :artefact_external_links do |t|
+      t.string :title
+      t.string :url
+      t.references :artefact, foreign_key: true
+      t.timestamps
+    end
+  end
+end