Create/update by NavigationLink identity (#16312)

Prior to this commit, we were treating all NavigationLink attributes as unique. So, were we to change a position of one of the NavigationLinks during the add_navigation_links rake task, we would have created a new NavigationLink (and the only one would have remained). With this commit, we're introducing the concept of the NavigationLink's surrogate identity, that is to say if we have two NavigationLink objects with the same `url` and `name` we should consider them the same NavigationLink. This allows us to update properties of those NavigationLinks (via the rake task) without the risk of creating new entries. This unblocks PR #16268 which addresses #16076.
2022-01-27 13:51:41 -05:00 · 2022-01-27 13:51:41 -05:00 · 0484c550c5
commit 0484c550c5
parent 1ae10d7e6f
3 changed files with 151 additions and 72 deletions
--- a/app/models/navigation_link.rb
+++ b/app/models/navigation_link.rb
@ -13,6 +13,41 @@ class NavigationLink < ApplicationRecord

  scope :ordered, -> { order(position: :asc, name: :asc) }

+  # With the given :url either create a NavigationLink or update an existing NavigationLink with the
+  # given :attributes.
+  #
+  # @param url [String] the URL of the navigation link.
+  # @param name [String] the name of the navigation link.
+  #
+  # @param attributes [Hash<Symbol, Object>] the attributes to create or update the given :url.
+  #        Note, this assumes hash keys.
+  #
+  # @return [NavigationLink]
+  #
+  # @note In constructing this function, the named args (e.g., :url and :name) are what we check for
+  #       "equality" with other NavigationLinks.  We can say that two NavigationLink object's that
+  #       have the same :name and :url are the same.  This way, when we update the NavigationLink's
+  #       position we ensure that we're not create new records (as per the past `first_or_create`
+  #       methodology.)
+  def self.create_or_update_by_identity(url:, name:, **attributes)
+    find_or_initialize_by(url: normalize_url(url), name: name).update(attributes)
+  end
+
+  # A helper function to ensure that we're normalizing URLs that we store (or query from storage).
+  #
+  # @param url [String]
+  #
+  # @return [String]
+  #
+  # @see NavigationLink.create_or_update_by_identity
+  # @see NavigationLink#strip_local_hostname
+  def self.normalize_url(url)
+    parsed_url = URI.parse(url)
+    return url unless url.match?(/^#{URL.url}/i)
+
+    parsed_url.path
+  end
+
  private

  # We want to allow relative URLs (e.g. /contact) for navigation links while
@ -27,9 +62,6 @@ class NavigationLink < ApplicationRecord
  # When persisting to the database we store local links as relative URLs which
  # makes it easier to switch from a forem.cloud subdomain to the live domain.
  def strip_local_hostname
-    parsed_url = URI.parse(url)
-    return unless url.match?(/^#{URL.url}/i)
-
-    self.url = parsed_url.path
+    self.url = self.class.normalize_url(url)
  end
 end
--- a/lib/tasks/add_navigation_links.rake
+++ b/lib/tasks/add_navigation_links.rake
@ -53,56 +53,56 @@ namespace :navigation_links do

  namespace :find_or_create do
    task readinglist: :environment do
-      NavigationLink.where(url: "/readinglist").first_or_create(
-        name: "Reading List",
+      NavigationLink.create_or_update_by_identity(
        url: URL.url("readinglist"),
+        name: "Reading List",
        icon: reading_icon,
        display_only_when_signed_in: true,
-        position: 0,
+        position: 1,
        section: :default,
      )
    end

    task contact: :environment do
-      NavigationLink.where(url: "/contact").first_or_create(
+      NavigationLink.create_or_update_by_identity(
        name: "Contact",
        url: URL.url("contact"),
        icon: contact_icon,
        display_only_when_signed_in: false,
-        position: 1,
+        position: 2,
        section: :default,
      )
    end

    task code_of_conduct: :environment do
-      NavigationLink.where(url: "/code-of-conduct").first_or_create(
+      NavigationLink.create_or_update_by_identity(
        name: "Code of Conduct",
        url: URL.url(Page::CODE_OF_CONDUCT_SLUG),
        icon: thumb_up_icon,
        display_only_when_signed_in: false,
-        position: 0,
-        section: :other,
-      )
-    end
-
-    task privacy: :environment do
-      NavigationLink.where(url: "/privacy").first_or_create(
-        name: "Privacy Policy",
-        url: URL.url(Page::PRIVACY_SLUG),
-        icon: smart_icon,
-        display_only_when_signed_in: false,
        position: 1,
        section: :other,
      )
    end

+    task privacy: :environment do
+      NavigationLink.create_or_update_by_identity(
+        name: "Privacy Policy",
+        url: URL.url(Page::PRIVACY_SLUG),
+        icon: smart_icon,
+        display_only_when_signed_in: false,
+        position: 2,
+        section: :other,
+      )
+    end
+
    task terms: :environment do
-      NavigationLink.where(url: "/terms").first_or_create(
+      NavigationLink.create_or_update_by_identity(
        name: "Terms of Use",
        url: URL.url(Page::TERMS_SLUG),
        icon: look_icon,
        display_only_when_signed_in: false,
-        position: 2,
+        position: 3,
        section: :other,
      )
    end
@ -114,97 +114,112 @@ namespace :navigation_links do
    domain = Rails.application&.initialized? ? Settings::General.app_domain : ApplicationConfig["APP_DOMAIN"]
    base_url = "#{protocol}#{domain}".freeze

-    NavigationLink.where(url: "#{base_url}/readinglist").first_or_create(
+    NavigationLink.create_or_update_by_identity(
+      url: "#{base_url}/readinglist",
      name: "Reading List",
      icon: reading_icon,
      display_only_when_signed_in: true,
-      position: 0,
-      section: :default,
-    )
-    NavigationLink.where(url: "#{base_url}/listings").first_or_create(
-      name: "Listings",
-      icon: listing_icon,
-      display_only_when_signed_in: false,
-      position: 1,
-      section: :default,
-    )
-    NavigationLink.where(url: "#{base_url}/pod").first_or_create(
-      name: "Podcasts",
-      icon: mic_icon,
-      display_only_when_signed_in: false,
      position: 2,
      section: :default,
    )
-    NavigationLink.where(url: "#{base_url}/videos").first_or_create(
-      name: "Videos",
-      icon: camera_icon,
+    NavigationLink.create_or_update_by_identity(
+      url: "#{base_url}/listings",
+      name: "Listings",
+      icon: listing_icon,
      display_only_when_signed_in: false,
      position: 3,
      section: :default,
    )
-    NavigationLink.where(url: "#{base_url}/tags").first_or_create(
-      name: "Tags",
-      icon: tag_icon,
+    NavigationLink.create_or_update_by_identity(
+      url: "#{base_url}/pod",
+      name: "Podcasts",
+      icon: mic_icon,
      display_only_when_signed_in: false,
      position: 4,
      section: :default,
    )
-    NavigationLink.where(url: "#{base_url}/code-of-conduct").first_or_create(
-      name: "Code of Conduct",
-      icon: thumb_up_icon,
-      display_only_when_signed_in: false,
-      position: 0,
-      section: :other,
-    )
-    NavigationLink.where(url: "#{base_url}/faq").first_or_create(
-      name: "FAQ",
-      icon: bulb_icon,
+    NavigationLink.create_or_update_by_identity(
+      url: "#{base_url}/videos",
+      name: "Videos",
+      icon: camera_icon,
      display_only_when_signed_in: false,
      position: 5,
      section: :default,
    )
-    NavigationLink.where(url: "https://shop.dev.to/").first_or_create(
+    NavigationLink.create_or_update_by_identity(
+      url: "#{base_url}/tags",
+      name: "Tags",
+      icon: tag_icon,
+      display_only_when_signed_in: false,
+      position: 6,
+      section: :default,
+    )
+    NavigationLink.create_or_update_by_identity(
+      url: "#{base_url}/faq",
+      name: "FAQ",
+      icon: bulb_icon,
+      display_only_when_signed_in: false,
+      position: 7,
+      section: :default,
+    )
+    NavigationLink.create_or_update_by_identity(
+      url: "https://shop.dev.to/",
      name: "DEV Shop",
      icon: shopping_icon,
      display_only_when_signed_in: false,
      position: 6,
      section: :default,
    )
-    NavigationLink.where(url: "#{base_url}/sponsors").first_or_create(
+    NavigationLink.create_or_update_by_identity(
+      url: "#{base_url}/sponsors",
      name: "Sponsors",
      icon: heart_icon,
      display_only_when_signed_in: false,
      position: 7,
      section: :default,
    )
-    NavigationLink.where(url: "#{base_url}/about").first_or_create(
+    NavigationLink.create_or_update_by_identity(
+      url: "#{base_url}/about",
      name: "About",
      icon: rainbowdev,
      display_only_when_signed_in: false,
      position: 8,
      section: :default,
    )
-    NavigationLink.where(url: "#{base_url}/privacy").first_or_create(
-      name: "Privacy Policy",
-      icon: smart_icon,
-      display_only_when_signed_in: false,
-      position: 1,
-      section: :other,
-    )
-    NavigationLink.where(url: "#{base_url}/terms").first_or_create(
-      name: "Terms of Use",
-      icon: look_icon,
-      display_only_when_signed_in: false,
-      position: 2,
-      section: :other,
-    )
-    NavigationLink.where(url: "#{base_url}/contact").first_or_create(
+    NavigationLink.create_or_update_by_identity(
+      url: "#{base_url}/contact",
      name: "Contact",
      icon: contact_icon,
      display_only_when_signed_in: false,
      position: 9,
      section: :default,
    )
+
+    NavigationLink.create_or_update_by_identity(
+      url: "#{base_url}/code-of-conduct",
+      name: "Code of Conduct",
+      icon: thumb_up_icon,
+      display_only_when_signed_in: false,
+      position: 1,
+      section: :other,
+    )
+
+    NavigationLink.create_or_update_by_identity(
+      url: "#{base_url}/privacy",
+      name: "Privacy Policy",
+      icon: smart_icon,
+      display_only_when_signed_in: false,
+      position: 2,
+      section: :other,
+    )
+    NavigationLink.create_or_update_by_identity(
+      url: "#{base_url}/terms",
+      name: "Terms of Use",
+      icon: look_icon,
+      display_only_when_signed_in: false,
+      position: 3,
+      section: :other,
+    )
  end
 end
 # rubocop:enable Metrics/BlockLength
--- a/spec/models/navigation_link_spec.rb
+++ b/spec/models/navigation_link_spec.rb
@ -3,6 +3,38 @@ require "rails_helper"
 RSpec.describe NavigationLink, type: :model do
  let(:navigation_link) { create(:navigation_link) }

+  describe ".create_or_update_by_identity" do
+    let(:attributes) { attributes_for(:navigation_link).except(:url, :id, :name).stringify_keys }
+    let(:name) { navigation_link.name }
+
+    # I want an existing navigation link, but don't want to apply the `let!` to the declaration as
+    # that impacts tests in other describe blocks.
+    before { navigation_link }
+
+    context "when the url already exists" do
+      let(:url) { navigation_link.url }
+
+      it "updates the existing NavigationLink" do
+        expect do
+          described_class.create_or_update_by_identity(url: url, name: name, **attributes.symbolize_keys)
+        end.not_to change(described_class, :count)
+
+        expect(navigation_link.reload.attributes.slice(*attributes.keys)).to eq(attributes)
+      end
+    end
+
+    context "when the url does not exist" do
+      # Creating a different URL
+      let(:url) { "#{navigation_link.url}-404" }
+
+      it "creates a new NavigationLink" do
+        expect do
+          described_class.create_or_update_by_identity(url: url, name: name, **attributes)
+        end.to change(described_class, :count).by(1)
+      end
+    end
+  end
+
  describe "validations" do
    describe "presence validations" do
      it { is_expected.to validate_presence_of(:name) }