Updates Readme sections for only and ignore

[tlynam]

Thank you for your contribution!

Check the following boxes:

• Wrote good commit messages.
• Feature branch is up-to-date with master (if not - rebase it).
• Squashed related commits together.
• Added tests.
• Added an entry to the Changelog if the new
  code introduces user-observable changes.
• The PR relates to only one subject with a clear title
  and description in grammatically correct, complete sentences.

[tlynam]

Hi @jaredbeck, hope you're well. It looks like ignore was only working for arrays. Ignore was working for hashes when calculating changed_and_not_ignored. Can we reuse this code for both cases? Once it's flushed out, I can add tests and update the changelog.

[jaredbeck]

I couldn't find documentation re: hashes in the :ignore array. Thankfully, we do have tests in spec/models/article_spec.rb. Based on my review of those tests, and events/base.rb, it seems like #1240 is not a bug report, but rather, is proposing a new feature.

Currently, we allow the :ignore array to contain hashes. By comparison, #1240 proposes that :ignore be a hash. Therefore, #1240 proposes a new feature.

I'm hesitant to increase the complexity of our public API in this way.

[jaredbeck]

Frankly, the current API, which allows the :ignore array to contain hashes, seems oddly complicated to me. I'd be open to simplifying it in our next major version. Perhaps :ignore should only accept an array of symbols, and a new option :ignore_if could take a Proc.

[tlynam]

Thank you @jaredbeck! Can you please help me understand this part of the documentation? To me, it reads like ignore can be a hash?

The :ignore and :only options can also accept Hash arguments.

class Article < ActiveRecord::Base
  has_paper_trail only: { title: Proc.new { |obj| !obj.title.blank? } }
end

[jaredbeck]

Ah, good find. But, looking at Events::Base#notably_changed, I think that documentation is incorrect?

paper_trail/lib/paper_trail/events/base.rb

Lines 221 to 235 in 21ead88

	def notably_changed
	# Memoized to reduce memory usage
	@notably_changed ||= begin
	only = @record.paper_trail_options[:only].dup
	# Remove Hash arguments and then evaluate whether the attributes (the
	# keys of the hash) should also get pushed into the collection.
	only.delete_if do |obj|
	obj.is_a?(Hash) &&
	obj.each { |attr, condition|
	only << attr if condition.respond_to?(:call) && condition.call(@record)
	}
	end
	only.empty? ? changed_and_not_ignored : (changed_and_not_ignored & only)
	end
	end

As with :ignore, I think that :only can contain hashes, but cannot be a hash.

So, now I'm not sure whether the docs are wrong or the implementation is wrong 😅 but I think they're different. 😓

[tlynam]

Ah, thank you! I see that now. Okay, would the simplest route be to update the documentation for ignore describing that it only works with an array of symbols? I like your idea of separating ignore and ignore_if. Would ignore_if be a hash of procs?

[jaredbeck]

.. would the simplest route be to update the documentation ..

Yeah, let's just update the docs for now.

.. [Should the docs] for ignore describing that it only works with an array of symbols?

Yes, but we should also give an example of an array of symbols that also contains a hash. See spec/dummy_app/app/models/article.rb. We may want to say that we are considering deprecating the later.

I like your idea of separating ignore and ignore_if. Would ignore_if be a hash of procs?

I was thinking it'd be a single proc. WDYT?

[tlynam]

Ah, thank you, I was confused by what you meant by ignore_if.

Okay, so the bug report is mentioning that a hash that's inside of an array isn't working as expected for ignore. It looks like it responds to a hash within an array for changed_and_not_ignored and was trying to extract and use this same logic for ignored_attr_has_changed? as well. It looks like ignored_attr_has_changed? may just work with an array without hashes. Do you see the same thing? Is it possible there isn't an existing test for ignore with a hash within an array?

[jaredbeck]

[is there] an existing test for ignore with a hash within an array?

Yes, spec/models/article_spec.rb.

I'd suggest that we focus, for now, on adding examples of current behavior to the docs (section 2.c.) and hold off on discussing any changes to the code until that's been done.

[tlynam]

I didn't see that we document this. Is it located somewhere else?

[tlynam]

Hi @jaredbeck, I hope you're well.

1. Okay, what do you think of these Readme updates saying we support ignore with an array of symbols and hashes with an example?
2. Do we need to document that we don't save a version row for timestamp attributes if the only updated column is ignored?
3. I believe I was able to reproduce the reported bug as a failing test in spec/models/gadget_spec.rb. It's saving a version record for updated_at when the updated column is ignored via Hash. This doesn't occur when the column is ignored via symbol. I believe the changes in lib/paper_trail/events/base.rb address it. I think changed_notably? would evaluate ignored_attr_has_changed? as false when it should be true and so it would create a version row for updated_at.
4. If you see this as a bug, should we address it?

[jaredbeck]

I may be misunderstanding, but I think this one line has already been said, above.

[tlynam]

Thank you, I missed that 👍

[jaredbeck]

Do we need to document that we don't save a version row for timestamp attributes if the only updated column is ignored?

Sure, but can you make it a separate PR please?

I believe I was able to reproduce the reported bug as a failing test in spec/models/gadget_spec.rb. It's saving a version record for updated_at when the updated column is ignored via Hash. This doesn't occur when the column is ignored via symbol. I believe the changes in lib/paper_trail/events/base.rb address it. I think changed_notably? would evaluate ignored_attr_has_changed? as false when it should be true and so it would create a version row for updated_at.
If you see this as a bug, should we address it?

I may be missing something, but I don't see the need to make any code changes. For now, I'm satisfied with correcting the documentation.

[tlynam]

Thank you @jaredbeck, this PR now just updates the Readme. I created a new PR with a failing test #1256

I also created a PR to fix CI and set the Rubocop gem to a fixed minor version so CI won't install a newer version that starts erroring in CI. It also updates the Rubocop gems. #1254