User id on order is always logged to spree_state_changes

[jasonfb]

I was examining the spree_state_changes table recently and noticed that the user_id on the state change record is always the order's user_id.

Indeed, it is clearly so, because it is set right here:
https://github.com/spree/spree/blob/master/core/app/models/spree/order/checkout.rb#L56

I had a specific request to record the Admin user who cancels an order. It would seem to me that if an Admin initiated anything that changes an order (Cacnelation, resuming, completing the order, etc), then that Admin user should be recorded as the user_id of the state change record, no?

If there is some compelling reason why this is not conceptually the case, then I would argue: why does the user_id field exist at all on the state_changes table -- as is, it contains no useful or distinctive information as AFAIK order do not switch from one user to another.

TBH: I find the state changes implementation to be somewhat of a relic. (As well, my spree_state_changes table is huge)

If it were up to me, I would replace the functionality with https://github.com/collectiveidea/audited, which has a few quirks but ultimately is more powerful and will allow for auditing on any (or none, configurable) field on the order object or related objects (including the change of state)

[JDutil]

I don't think there is a compelling reason it's probably just some legacy code or not thought out well. I think changing to something like audited gem makes sense, but would probably prefer PaperTrail https://github.com/airblade/paper_trail as it's more popular & full featured for things like going back in history. close:question

[spreebot]

Please use the mailing list if you have questions about Spree that are not reporting a bug.

We simply cannot respond efficiently to questions through our Issue Tracker.

[jasonfb]

Yes ok cool thanks for the feedback. I understand.

in fact in my app I started with Paper trail and completely backed out of it because of two problems:

1. It created a completely unmanageable & massive data table that mushroomed in size well beyond the size of the rest of my database. Keeping it would have cost us so much $ in database hosting cost it wasn't worth it. The data was really that big (sorry I don't have numbers), and I found acts_as_audited to keep much smaller amount of data (I think this is because acts_as_audited records only the fields that change, whereas as papertrail records a snapshot of all fields on the object)

2. Papertrail was incompatible with Spree due to a name collision of the word "originator". I filed an issue with PT here a while ago: originator is a bad choice for method name paper-trail-gem/paper_trail#479

It looks like they have fixed that in later versions of PT, so may not be a problem

Personally, having worked with both, I actually prefer acts_as_audited as I find it is cleaner. As well one of the significant limitations of papertrail was that there wasn't an easy to way to see just what changed. You can see two different versions, and you can load them objects, but then if you want to see what changed you have to write your own comparison code which seems completely whack to me.

I find that Papertrail is strong for developers who want to work with objects and an API that has methods like .previous_version, .next_version, .reify, etc. However, I find acts_as_audited better suited for non-developer Admin users, who have a basic understanding of databases and really only care about actually examination of the audit history (with my human eyes, not with Ruby).

Building a 'change history' screen (like on my Users admin, where I think it is needed) -- where I can see the history of field-level changes change-by-change, is actually may be much easeir with acts-as-audited. In comparrison, because papertrail has this concept of keeping a snapshot of each version, I don't even know how to do that with papertrail. (see updates below)

Personally, I think that the latter (Admin users) are actually more important than the Developer users, so that's why in my app (in which this functionality has been added as custom) we've gone with acts_as_audited over PT.

That was a long explanation in response to the acts-as-audited vs. PT question.

This github issue is correctly closed-- it was indeed a question and not a bug. No further action needed here as my question has been answered.

[jasonfb]

Here's a nice clean proof-of-concept I have implemented in my app using acts_as_audited. As you can see, it has a nice clear picture of what fields changed when.

I looks like this actually may be possible with Papertrail also, but it is not configured out of the box to do this.

Here is a discussion of creating a field-level audit history using Papertrail gem: http://stackoverflow.com/questions/18925408/create-an-audit-trail-using-paper-trail-gem

as I said, this is created using acts_as_audited:

the code here is very simple (shown here in HAML)

  - @user.audits.each do |audit|
    %tr
      %td
        = audit.created_at
      %td
        = audit.try(:user).try(:email)
      %td
        %ul
          = raw(audit.audited_changes.collect{|k,v| "<li>changed #{k} from #{v[0]} to #{v[1]} </li>".html_safe}.join(""))
      %td
        = audit.remote_address

[jasonfb]

One final thought on this @JDutil (Sorry for all the comments-- feel free to get back to this if you are busy now):

After considering it a bit, because of the bloat issue associated with keeping database tables --- as well as our disagreement over acts_as_audited vs. PT ---- I actually think this is a good candidate to be removed from Core and spun out into its own Spree extension.

That way, the ultimate goal could be to have two separate extensions --- one for AAA and another for PT --- and we could let the developer choose which to use.

As well, because PT really does create a HUGE table (it really is way larger than it should be)--- and acts as audited also can create create a large audits table (not as big as Papertrail) --- I think this functionality should be "off" (or not in Core at all) by default. It makes sense to me for it to be an opt-in functionality, so that means it is either in Core and there's a setting for it, or it could simply be a Spree extension.

Assuming you + I won't agree on the acts-as-audited vs. PT question--- will you agree with me that the ultimate goal is that this can get removed from Core and spun out into a Spree extension? If we see eye-to-eye on that, I will volunteer to create the extension for using acts-as-audited (as I have most of the code already done in my app)

[JDutil]

I don't really have a strong opinion either way whether to convert to using acts_as_audited in core or making an extension as I don't regularly develop stores anymore after leaving Rails Dog and shutting mine down. I know some people use the state changes so I don't think we should remove the feature, but I don't have a problem with improving it to use acts_as_audited so long as we're not losing functionality. I think your right that PT is overkill since I don't think we want people to start reverting versions of orders and such I think that would end up giving people a tool they probably wouldn't know how to use properly, and open up new bugs/problems.

[jasonfb]

OK cool. I still think that the state_changes (existing functionality) should have at least a switch so it can be turned off... this bug makes it not that useful, since you can't see who or what initiated state changes

I also think that the acts as audited stuff could be its own Extension-- its not necessarily needed by everyone and conceptually it can work orthogonally to Spree so it seems like a good candidate to be an extension and not in core.

Auditing the order is somewhat interesting (but I wouldn't audit every field), but actually what I really want to audit is the User table.

As hackers become more sophisticated, we are seeing some users accounts with simple passwords get hijacked via dictionary attacks. At the very least, auditing gives us a record of the old email address of our customer before the hacker switches the account to a fake email address.

[JDutil]

I'm alright with a preference for turning it on/off as well, but I'm hesitant to change any existing functionality. I have no way of knowing who are using features, and who aren't using them. All I have is my past experience of what I or clients did or didn't use. As I mentioned I'm not working on Spree stores daily anymore so my opinion on things doesn't really matter as much as the people who are working with this software every day. So without the community debating and making a decision on these sorts of things I'm going to tend to side on not changing anything, otherwise I'll end up making someone upset that I changed something they were using several months later when they finally upgrade.