Prevent updates to Facebook count on non-canonical locations

[chrismaddocks]

No description provided.

[chrismaddocks]

Prevent Wakes From Updating Facebook Count On Legacy Locations

[benhutton]

@chrismaddocks what if, instead of all this, we redefined #facebook_count= to include a check for canonical? before setting the value? (And if not canonical, return some sort of warning.) Something like,

def facebook_count=(facebook_count)
  if canonical?
    super
  else
    'Cannot update the facebook count of a noncanonical location.'
  end
end

[chrismaddocks]

@benhutton The issue then is that facebook_count_updated_at will be updated even though facebook_count is not: https://github.com/desiringgod/wakes/blob/develop/app/services/wakes/facebook_count_updater_service.rb#L14.

While we could code around that, that smells like we want the transaction to fail, not the individual attribute (especially since we can't predict how future developers will try to set the attribute). Doesn't validating on save feel more aligned with ActiveModel, too?

[benhutton]

okay...

[chrismaddocks]

@benhutton You sound like you still have some hesitation. Let's talk...

[benhutton]

no. my ellipsis was merely to indicate that I was moving my questioning to the throw :abort line

[benhutton]

@chrismaddocks tell me more about throw :abort. Is that a standard / documented way of doing this?

[chrismaddocks]

@benhutton Yes, this is the Rails 5 way of halting the callback chain when a validation fails (returning false no longer works).

[benhutton]

gotcha. that's helpful

[chrismaddocks]

@gaganawhad I had submitted this several months ago, but looks like I never merged it. I see you solved a related problem in #49. What do you think of the refactor in 8c6e430? The advantage of doing it with filters is that bad updates will be caught whether they use your update_facebook_count method or try to update the attribute directly.

[gaganawhad]

@chrismaddocks I like the refactor. One point that's not really related with your refactor is, I wonder if we want to keep a possibility of allowing updates for decreasing counts when we are forcing a decreasing count for error correction. We have had to do that a few times in the past. May be something we don't need to really bother about in code.

[chrismaddocks]

@gaganawhad One more commit (8e6fdbd) needed here to get the specs to pass, which may be a smell for the way we are trying to do atomic updates here. Maybe you have another idea here? I've also assigned you as a reviewer.

Also, the tests will fail until #55 is merged.