[15 min fix] Refactor private landing pages model code (#13982)

* Add LandingPage domain model

* Rely on Rails data integrity and remove previous code

* Remove superfluous equality check

* Restore landing_page class method

* Add and fix specs

* Do not attach landing page modal controller unnecessarily

app/controllers/admin/pages_controller.rb

@@ -10,7 +10,8 @@ module Admin
     end
 
     def new
-      @landing_page = Page.find_by(landing_page: true)
+      @landing_page = Page.landing_page
+
       if (slug = params[:slug])
         prepopulate_new_form(slug)
       else
@@ -20,12 +21,13 @@ module Admin
 
     def edit
       @page = Page.find(params[:id])
-      @landing_page = Page.find_by(landing_page: true)
+      @landing_page = Page.landing_page
     end
 
     def update
       @page = Page.find(params[:id])
-      if update_and_overwrite_landing_page
+
+      if @page.update(page_params)
         flash[:success] = "Page has been successfully updated."
         redirect_to admin_pages_path
       else
@@ -36,7 +38,8 @@ module Admin
 
     def create
       @page = Page.new(page_params)
-      if create_and_overwrite_landing_page
+
+      if @page.save
         flash[:success] = "Page has been successfully created."
         redirect_to admin_pages_path
       else
@@ -48,6 +51,7 @@ module Admin
     def destroy
       @page = Page.find(params[:id])
       @page.destroy
+
       flash[:success] = "Page has been successfully deleted."
       redirect_to admin_pages_path
     end
@@ -55,8 +59,10 @@ module Admin
     private
 
     def page_params
-      allowed_params = %i[title slug body_markdown body_html body_json description template is_top_level_path
-                          social_image landing_page overwrite_landing_page]
+      allowed_params = %i[
+        title slug body_markdown body_html body_json description template
+        is_top_level_path social_image landing_page
+      ]
       params.require(:page).permit(allowed_params)
     end
 
@@ -95,31 +101,5 @@ module Admin
                 Page.new
               end
     end
-
-    def update_and_overwrite_landing_page
-      if page_params["overwrite_landing_page"] == "true"
-        Page.transaction do
-          current_landing_page = Page.find_by(landing_page: true)
-          current_landing_page&.update(landing_page: false)
-
-          @page.update(page_params)
-        end
-      else
-        @page.update(page_params)
-      end
-    end
-
-    def create_and_overwrite_landing_page
-      if page_params["overwrite_landing_page"] == "true"
-        Page.transaction do
-          current_landing_page = Page.find_by(landing_page: true)
-          current_landing_page&.update(landing_page: false)
-
-          @page.save
-        end
-      else
-        @page.save
-      end
-    end
   end
 end

app/controllers/application_controller.rb

@@ -54,11 +54,9 @@ class ApplicationController < ActionController::Base
     return if self.class.module_parent.to_s == "Admin"
     return if user_signed_in? || Settings::UserExperience.public
 
-    @page = Page.landing_page
-
     if api_action?
       authenticate!
-    elsif @page
+    elsif (@page = Page.landing_page)
       render template: "pages/show"
     else
       @user ||= User.new

app/javascript/admin/controllers/landing_page_modal_controller.js

@@ -1,7 +1,7 @@
 import ModalController from './modal_controller';
 
 export default class LandingPageModalController extends ModalController {
-  static targets = ['overwrite', 'landingPageCheckbox'];
+  static targets = ['landingPageCheckbox'];
 
   openModal() {
     if (this.landingPageCheckboxTarget.checked) {
@@ -12,7 +12,6 @@ export default class LandingPageModalController extends ModalController {
   confirm(event) {
     event.preventDefault();
 
-    this.overwriteTarget.value = true;
     this.closeModal();
   }
 
@@ -20,7 +19,6 @@ export default class LandingPageModalController extends ModalController {
     event.preventDefault();
 
     this.landingPageCheckboxTarget.checked = false;
-    this.overwriteTarget.value = false;
     this.closeModal();
   }
 }

app/models/page.rb

@@ -1,6 +1,4 @@
 class Page < ApplicationRecord
-  attr_accessor :overwrite_landing_page
-
   TEMPLATE_OPTIONS = %w[contained full_within_layout json].freeze
 
   validates :title, presence: true
@@ -9,15 +7,20 @@ class Page < ApplicationRecord
   validates :template, inclusion: { in: TEMPLATE_OPTIONS }
   validate :body_present
   validate :unique_slug_including_users_and_orgs, if: :slug_changed?
-  validate :single_landing_page, if: :will_save_change_to_landing_page?
 
   before_validation :set_default_template
   before_save :evaluate_markdown
-  after_save :bust_cache
+
+  after_commit :ensure_uniqueness_of_landinge_page
+  after_commit :bust_cache
 
   mount_uploader :social_image, ProfileImageUploader
   resourcify
 
+  def self.landing_page
+    find_by(landing_page: true)
+  end
+
   def path
     is_top_level_path ? "/#{slug}" : "/page/#{slug}"
   end
@@ -26,10 +29,6 @@ class Page < ApplicationRecord
     "page_#{slug}"
   end
 
-  def self.landing_page
-    find_by(landing_page: true)
-  end
-
   private
 
   def evaluate_markdown
@@ -63,15 +62,13 @@ class Page < ApplicationRecord
     errors.add(:slug, "is taken.")
   end
 
-  def single_landing_page
-    # Only add errors if we are trying to modify a landing page
-    # while another landing page is already being used to ensure
-    # that only one can be set to "true" at a time.
+  # As there can only be one global landing page, we want to ensure that
+  # data integrity is preserved by setting `landing_page` to `false` for all
+  # other pages if the current one was transformed into a landing page
+  def ensure_uniqueness_of_landinge_page
+    return unless previous_changes["landing_page"] == [false, true]
 
-    landing_page = Page.where.not(id: id).find_by(landing_page: true)
-    return unless landing_page
-
-    errors.add(:base, "Only one page at a time can be used as a 'locked screen.'")
+    Page.where.not(id: id).update_all(landing_page: false)
   end
 
   def bust_cache

app/views/admin/pages/_form.html.erb

@@ -1,9 +1,14 @@
+<% if @landing_page %>
 <div class="crayons-card p-6"
      data-controller="landing-page-modal"
      data-landing-page-modal-root-selector-value="#add-landing-page-link-modal-root"
      data-landing-page-modal-content-selector-value="#add-landing-page-link-modal"
      data-landing-page-modal-title-value="There's another locked screen..."
      data-landing-page-modal-size-value="default">
+<% else %>
+<div class="crayons-card p-6">
+<% end %>
+
   <%= form_for [:admin, @page] do |form| %>
     <div class="form-group">
       <%= form.label :title %>
@@ -54,7 +59,6 @@
                                           "landing-page-modal-target": "landingPageCheckbox",
                                           action: (@landing_page.present? ? "landing-page-modal#openModal" : "")
                                         } %>
-      <%= form.hidden_field :overwrite_landing_page, data: { "landing-page-modal-target": "overwrite" } %>
       <p id="lock-screen-description">Determines if this page will be used as a landing page for anonymous viewers.</p>
     </div>
 

spec/models/page_spec.rb

@@ -36,13 +36,6 @@ RSpec.describe Page, type: :model do
       expect(page).not_to be_valid
       expect(page.errors[:slug].to_s.include?("taken")).to be true
     end
-
-    it "only allows a single landing_page to be set to true" do
-      create(:page, landing_page: true)
-      page = build(:page, landing_page: true)
-      expect(page).not_to be_valid
-      expect(page.errors[:base].to_s.include?("Only one page")).to be true
-    end
   end
 
   context "when callbacks are triggered before save" do
@@ -62,7 +55,7 @@ RSpec.describe Page, type: :model do
     end
   end
 
-  context "when callbacks are triggered after save" do
+  context "when callbacks are triggered after commit" do
     let(:page) { create(:page) }
 
     it "triggers cache busting on save" do
@@ -70,5 +63,21 @@ RSpec.describe Page, type: :model do
         page.save
       end
     end
+
+    it "ensures only one page can be a landing page on create" do
+      p1 = create(:page, landing_page: true)
+      create(:page, landing_page: true)
+
+      expect(p1.reload.landing_page).to be(false)
+    end
+
+    it "ensures only one page can be a landing page on update" do
+      p1 = create(:page, landing_page: true)
+      p2 = create(:page, landing_page: false)
+
+      p2.update(landing_page: true)
+
+      expect(p1.reload.landing_page).to be(false)
+    end
   end
 end