magnusvk/counter_culture

Multi-level counter-cache doesn't work with Discard

Opened this issue · 4 comments

It seems that couter_culture checks Discard of only one level of models hierarchy, but not all of them.

For example:

class Step < ActiveRecord::Base
  include Discard::Model

  belongs_to :lesson, class_name: 'Lesson'

  counter_culture %i[lesson unit]
  counter_culture :lesson
end


class Lesson < ActiveRecord::Base
  include Discard::Model

  belongs_to :unit
  has_many :steps
end


class Unit < ActiveRecord::Base
  include Discard::Model

  has_many :lessons
  has_many :steps, through: :lessons
end
u = Unit.create

l1 = u.lessons.create
l2 = u.lessons.create

l1.steps.create
l2.steps.create

u.reload.steps_count # = 2, correct

l1.discard

u.reload.steps_count # = 2, wrong, but okay

Step.counter_culture_fix_counts

## Generated SQL request, 'steps.discarded_at IS NULL' is checked, but 'lessons.discarded_at IS NULL' is not
#  Unit Load (3.1ms)  SELECT "units".id, "units".id, COUNT("steps".id)*1 AS count, MAX("units".steps_count) AS steps_count FROM "units" LEFT JOIN "lessons" AS lessons ON "units".id = lessons.unit_id LEFT JOIN "steps" AS steps ON "lessons".id = steps.lesson_id AND steps.discarded_at IS NULL GROUP BY "units"."id" ORDER BY "units"."id" ASC LIMIT $1  [["LIMIT", 1000]]
#  Lesson Load (1.3ms)  SELECT "lessons".id, "lessons".id, COUNT("steps".id)*1 AS count, MAX("lessons".steps_count) AS steps_count FROM "lessons" LEFT JOIN "steps" AS steps ON "lessons".id = steps.lesson_id AND steps.discarded_at IS NULL GROUP BY "lessons"."id" ORDER BY "lessons"."id" ASC LIMIT $1  [["LIMIT", 1000]]

u.reload.steps_count # = 2, still wrong

Expected:

Step.counter_culture_fix_counts also takes into account discarded_at of parent model.

Actual:
Step.counter_culture_fix_counts checks only discarded_at of Step, but any model on hierarchy.

For now we mitigated the issue with custom scope:

class Step < ActiveRecord::Base
  include Discard::Model

  belongs_to :lesson, class_name: 'Lesson'
  scope :within_kept, -> { kept.where(lesson_id: Lesson.kept.select(:id)) }

  counter_culture %i[lesson unit], column_name: ->(_) { 'steps_count' },
                    column_names: {
                      Step.within_kept => :steps_count
                    }
  counter_culture :lesson
end

This sounds right—do you think you can put a pull request together to fix this? I don't use discard so wouldn't get around to fixing this myself.

Was this ever merged? Or do I use the above solution?

Nobody ever put together a PR to fix this, so there was nothing to merge. I don't have the time to work on this myself. If you can put together a PR for this I'd be happy to merge it!