Skip to content

Commit

Permalink
Fix a bug where specialist sectors wouldn't stick
Browse files Browse the repository at this point in the history
Because this code was using `defined?` to check for the presence of the `specialist_sectors` method on the edition, and the edition is wrapped in a `LocalisedModel` which technically doesn't have that method defined, the code was replacing existing specialist sectors with recommended ones.

Using `respond_to?` provides the correct behaviour since it accounts for `method_missing` in `LocalisedModel` and therefore uses duck typing to figure out whether it's safe to call `specialist_sectors` on the object.
  • Loading branch information
elliotcm committed Mar 5, 2019
1 parent e2b2f38 commit af8848b
Show file tree
Hide file tree
Showing 2 changed files with 46 additions and 12 deletions.
2 changes: 1 addition & 1 deletion app/services/taxons_to_legacy_associations_tagging.rb
Expand Up @@ -49,6 +49,6 @@ def legacy_mapping_for_taxons(selected_taxons)
end

def tagged_to_specialist_sectors?(edition)
defined?(edition.specialist_sectors) && edition.specialist_sectors.any?
edition.respond_to?(:specialist_sectors) && edition.specialist_sectors.any?
end
end
56 changes: 45 additions & 11 deletions test/unit/services/taxons_to_legacy_associations_tagging_test.rb
@@ -1,28 +1,62 @@
require 'test_helper'

class TaxonsToLegacyAssociationsTaggingTest < ActiveSupport::TestCase
test "handles specialist sectors" do
specialist_sector_content_id = SecureRandom.uuid
taxon = taxon_mapped_to_specialist_sector(
specialist_sector_content_id
setup do
@specialist_sector_content_id = SecureRandom.uuid
@taxon = taxon_mapped_to_specialist_sector(
@specialist_sector_content_id
)

Taxonomy::TopicTaxonomy
.any_instance
.stubs(:all_taxons)
.returns([taxon])
.returns([@taxon])

@edition = create(:publication)
@edition.topics.delete_all
end

test "handles specialist sectors" do
TaxonsToLegacyAssociationsTagging.new.call(
edition: @edition,
user: create(:user),
selected_taxons: [@taxon.content_id]
)

assert @edition.specialist_sectors.count, 1
assert @edition.specialist_sectors.first.topic_content_id, @specialist_sector_content_id
end

edition = create(:publication)
edition.topics.delete_all
test "it doesn't set specialist sectors if they're already set" do
specialist_sector = FactoryBot.create(:specialist_sector,
edition: @edition,
topic_content_id: SecureRandom.uuid,
)

TaxonsToLegacyAssociationsTagging.new.call(
edition: @edition,
user: create(:user),
selected_taxons: [@taxon.content_id]
)

assert_equal(1, @edition.specialist_sectors.count)
assert_equal(specialist_sector, @edition.specialist_sectors.first)
end

test "it doesn't set specialist sectors if they're already set and the edition is wrapped in a LocalisedModel" do
specialist_sector = FactoryBot.create(:specialist_sector,
edition: @edition,
topic_content_id: SecureRandom.uuid,
)

TaxonsToLegacyAssociationsTagging.new.call(
edition: edition,
edition: LocalisedModel.new(@edition, @edition.primary_locale),
user: create(:user),
selected_taxons: [taxon.content_id]
selected_taxons: [@taxon.content_id]
)

assert edition.specialist_sectors.count, 1
assert edition.specialist_sectors.first.topic_content_id, specialist_sector_content_id
assert_equal(1, @edition.specialist_sectors.count)
assert_equal(specialist_sector, @edition.specialist_sectors.first)
end

def taxon_mapped_to_specialist_sector(specialist_sector_content_id)
Expand Down

0 comments on commit af8848b

Please sign in to comment.