Change from non-existent to false not being tracked

[BrianLMatthews]

In a model I have:

  field :exclusion, :type => Boolean
...
  track_history on: :all, modifier_field_inverse_of: false

When the document is initially created, the exclusion field doesn’t exist. Later it gets set, and if it’s set to false, the change isn’t tracked. I’ve tracked things down to line 29 in lib/mongoid/history/attributes/update.rb:

                { k => format_field(k, v) } unless v.all?(&:blank?)

It’s the v.all that’s the problem. v at that point is [nil, false], and both nil and false are blank? so it’s not tracking that change. While that’s clearly on purpose it seems wrong. Changing something from non-existent to false is a change (mongoid thinks so or it wouldn’t be in changes), so it seems it should be tracked (assuming all other things say it should be tracked). Thoughts?

[dblock]

Looks like a bug. Want to try to write a (failing) spec for it and maybe a fix?

[BrianLMatthews]

No, it’s actually on purpose (at least going by it being tested for 😆). According to the test (starting on spec/unit/attributes/update_spec.rb:308) it’s for the case of changing a has_and_belongs_to_many field from nil to [], although it then also ignores changing a Boolean from nil to false, a String from nil to “”, a Hash from nil to {}, etc. If I remove the unless v.all?(&:blank?) on lib/mongoid/history/attributes/update.rb:29 that test fails (as expected). I’m not familiar enough with mongoid to know if that’s a common case that one wouldn’t want to track, but offhand I’d say if mongoid thinks it’s a change it should be tracked, but could easily be wrong about that. The undo stuff would also have to be looked at to see what it would do with a nil -> something blank? change (presumably just change the field to nil, but my app doesn’t use that so I haven’t tried it).

[dblock]

if mongoid thinks it’s a change it should be tracked

I completely agree with you. For backwards compatibility I would introduce an option (track_nil: true/false?). If you have time, start by writing specs for the many types that all fail, then we can have some confidence when introducing this.

[johnnyshields]

@dblock I think it's safe to change the &:blank? logic here to &:nil? without introducing a new config. A slightly more conservative approach could be:

• v.blank? && !v.is_a?(FalseClass) OR
• v.nil? || v.try(:empty?)

Either of the above would still consider empty array values as "blank" which I think is desirable here.

[dblock]

@johnnyshields Possibly. As long as no existing specs fail and we document it thoroughly.

[BrianLMatthews]

Just dropping the unless v.all?(&:blank?) would make one spec fail, the one I pointed out in my previous message. I tend to be conservative in making changes so I’d add a config option (although I’d probably call it track_all_blank or something), but it’s not my code so really @dblock’s choice.

On using &:nil? instead of &:blank?, at that point v is an exactly two-element array of [<old value>, <new value>], obtained from calling changes then iterating over all the entries in the returned Hash. So v can never be [nil, nil]. Similarly, a test for false would miss things like [nil, ""]. I got started on this path because of [nil, false], but any [<blank thing>, <different blank thing>] will also not be tracked even though it appears in changes.

[johnnyshields]

My take is:

• nil --> false = log the change
• nil --> "" = skip
• nil --> [] = skip
• nil --> nil = skip (obviously)

I think this can probably be done without a config flag, or if we do add one, deprecate it and remove it in the next major version.

[BrianLMatthews]

I’m not sure why you would want to track just some changes. In my case I definitely want the first two tracked, and maybe the 3rd, although I’d have to look at where we use such fields to be sure (and if I don’t need it tracked, that also means it won’t hurt anything if it’s suddenly tracked). But really, I don’t think mongoid-history should be making any implicit assumptions about what someone may or may not want tracked (except _id and the time tracking fields). It can’t know all use cases, and users already have explicit coarse-grained and fine-grained control with :on and :changes_method.

[johnnyshields]

In my case it's more about showing the changes to the end users. I'd prefer to skip trivial changes, it's just noise for me.

[BrianLMatthews]

Which kind of makes my point. It’s noise to you, it’s definitely not to me. mongoid-history has no way to know our different needs so shouldn’t be making those kinds of assumptions, it should support both use cases IMO.

[dblock]

I say let's see a PR from @BrianLMatthews and we can discuss whether to merge/not merge it?

[BrianLMatthews]

Before I do a PR I’d like to at least agree on a general approach. My suggestion (and what I’d do in a PR) would be:

1. Add a track_all_blank option (not married to the name) defaulting to false.
2. On lib/mongoid/history/attributes/update.rb:29 don’t do the unless if the new option is true.

So it will work as now out of the box. If someone adds :track_all_blank => true to a track_history call, changes where both members of a change pair respond with true to blank? will now be tracked (or more accurately won’t not be tracked).

If that’s ok I’ll start on a PR (it won’t be done immediately, pesky real job and such). I’ve read through CONTRIBUTING.md and will do all that.

[johnnyshields]

@BrianLMatthews this looks fine, I think should separate "False" from "Blank" in this context.

[dblock]

Naming-wise I think track_blank is simpler and clearer I think.

Semantically this is incorrect, but I can't come up with a better option. "Track blank" means "track blank change history", but nil and [] are both blank, so blank? doesn't change and therefore has no history. Default it to true would be even weirder, and I don't like track_nil though because we're not really tracking nils only. 🤷‍♂️

[BrianLMatthews]

I think should separate “False” from “Blank” in this context.

I see that as a separate issue. I, personally, don’t think mongoid-history should be doing any implicit filtering of changes, and certainly not any that can’t (easily) be turned off by the user. So I’d like to keep this issue just allowing the current implicit filtering to be turned off. Any changes to that filtering would be a separate issue, although I will note that once the implicit filtering can be turned off, any filtering desired can be handled by a custom changes method (something that can’t be done today).

Naming-wise I think track_blank is simpler and clearer I think.

I was thinking about the naming of the option, as I’m not happy with what I suggested either. I think the problem though is that it’s a “reversed” option, i.e. false means to turn something on and true means to turn something off, which I always find confusing, and is a big no no in UI design. So I’d like to suggest the option be ignore_all_blank_changes with a default of true. That matches current behavior, and if someone sets it to false, it turns off the implicit filtering. That seems clearer to me as true is now turning on some behavior. The default has to be true for backwards compatibility.

[johnnyshields]

I would vote to call it track_blank_changes with default false. I think it makes sense to be able to set a global default for this.

[dblock]

I'm good with what @johnnyshields is suggesting, possibly as a start ;)

[BrianLMatthews]

Ok, I'll start on a PR.

[BrianLMatthews]

#257