Feedback Round 1

[ryantownsend]

spec/models/admin_spec.rb

General

• I believe it'll be better to test this stuff against the update_slug_prefixes_for_active_slugs service instead, this will mean it's easier to make the callback asynchronous and just simply test that when you change a prefix, it queues a worker.

L9-L11

• use create_list(:product, 2)
• always best to limit number of records to most minimal number to validate test, performance adds up across many specs

L15-31

• Wrap each in aggregate_failures block, means it'll output all failures, not just one spec spec then stop, better for debugging a collection of changes

spec/models/product_spec.rb

General

• I wouldn't add this to a specific model spec and move to a spec surrounding the sluggable concern, e.g. I'd have a file called something like sluggable_spec.rb
• I would create a shared example which simply tests if the Sluggable module is included, e.g. it_behaves_like 'a sluggable model'. This way 1) we've centralised the spec against the relevant module and 2) the model specs are rapid

L11-14

• Appreciate you're testing the factory works here, but given it would break elsewhere too, I'd remove this spec

L17, L23, L29

• These won't print particularly useful output, I'd have each in an individual context, rather than one general context around the 3 spec, e.g. 'with a snake_case_slug', 'with a kebab-case-slug' and 'with a slug/containing/folders'

L93, L99, L104, as per app/models/slug.rb below

• Refactor to use named scopes e.g. subject.slugs.active.first, means the implementation of active is masked from the spec

L37, L42, L47, L52, L57

• Same as above, I'd move to individual context with more specific description

app/models/admin.rb

L2, L6

• I would call this update_active_slug_prefixes to be more explicit with intent

L7

• I would consider pass the prefix into this service to decouple the service from knowing where and how the prefix is stored.
• Make asynchronous so large slug sets don't slow down the HTTP response or timeout

app/models/slug.rb

General

• Add active and inactive named-scopes
• Add self.active_for_resource_type(resource_type); active.where(resource_type: resource_type); end method, this treats the AR model as more of a repository rather than chaining AR scopes outside the class too much

L2

• Remember for Rails <5 you need to add a validation for belongs_to associations

app/services/slugs/update_slug_prefixes_for_active_slugs.rb

L4, as per app/models/admin.rb L7 above, and L34 below

• Add slug_prefix to arguments
• Add slug_computer to arguments, with a default of Slugs::ComputeSlug to allow for dependency injection in your specs

L14,L15

• Update to use find_in_batches, e.g. Slug.active_for_resource_type(resource_type).find_in_batches do |batch| batch.each do |slug| ... this will lower memory usage

L25

• I don't think this is the best name, generate_attributes would be more consistent with the output, however if we reimplement as below, then it will be generating a record, so we can leave it
• This seems quite complex, may be better as:

active_slug.dup.tap { |s| s.assign_attributes(slug_params(s)) }

You will just need to add active: true to slug_params. Then L17-L18 becomes:

generate_record(active_slug).save!

L34, as per L4 above and app/services/slugs/compute_slug.rb L2/L6/L8 below

• Replace direct Slugs::ComputeSlug.call with slug_computer.call to allow for dependency injection in your specs
• Replace passing resource_type with passing slug_prefix from this class (as we pass in as an argument)

app/services/slugs/compute_slug.rb

L2, L6, L8

• Replace resource_type with slug_prefix, both places this service is implemented, we know where the prefix comes from

app/models/concerns/sluggable.rb

L25

• I think after_save can be combined with a rollback to give us what we want in a cleaner way (not needing to use .build for the new slug)

L43

• Update to use active named scope

L46-L56

• Migrate this to a service, e.g. Slugs::UpdateSluggableHistory, this means we can use dependency injection to pass in the Slugs::ComputeSlug service used in L62

L47

• transaction is an instance method on AR models, not just a class method so you can replace self.class.transaction do with just transaction do

L61

• Given how we're using this method on L29, this may be better named computed_slug

L62

• As with app/services/slugs/compute_slug.rb L2/L6/L8, replace resource_type with slug_prefix

So ultimately, compute_slug is removed and update_slug_history becomes:

after_save :update_slug_history # this still allows for rolling-back the transaction

def update_slug_history
  unless Slugs::UpdateSluggableHistory.call(slug_prefix: self.class.slug_prefix, resource: self)
    self.errors.add(:slug, 'already taken')
    throw :abort # Rails 5 only
    raise ActiveRecord::RecordInvalid # Rails 4, or may be: raise ActiveRecord::Rollback
  end
end

def computed_slug # note: no longer compute_slug
  Slugs::ComputeSlug.call(slug_prefix: slug_prefix, slug: resource.slug)
end

class Slugs::UpdateSluggableHistory
  def self.call(*args)
    new(*args).call
  end

  def initialize(slug_prefix:, resource:)
    @slug_prefix = slug_prefix
    @resource = resource
  end

  def call
    @resource.slugs.active.update_all(active: false) &&
    @resource.slugs.active.create(
      slug_prefix: @slug_prefix, 
      slug: @resource.slug, 
      computed_slug: @resource.computed_slug
    )
  end
end

[krisquigley]

Looking through this again, my previous concerns were not relevant. Should have this done today.

[krisquigley]

@resource.slugs.active.create(
slug_prefix: @slug_prefix,
slug: @resource.slug,
computed_slug: @resource.computed_slug
)

Unable to call #create here as the parent hasn't been saved yet, reverted to build