Exclude canceled orders from sales report

[brchristian]

As I mentioned in issue #2099, by default the Sales Total report does not exclude canceled orders from the sales totals. This is the "minimum viable PR" to fix that issue.

I’m open to further discussion from the Solidus community around the following:

1. Should we make proper canceled and not_canceled scopes for the Order model, to go along with frequently used scopes like complete?

2. Should the complete scope by default exclude canceled orders unless explicitly called for complete_including_canceled, the way that Product#variants by default excludes the master variant unless and we specify variants_including_master, or how various scopes exclude deleted objects unless we use with_deleted scopes?

If "yes" to either (1) or (2), chime in on this thread and I’m happy to whip up another easy PR.

[mamhoff]

1. Should we make proper canceled and not_canceled scopes for the Order model, to go along with frequently used scopes like complete?

Yes, that would be nice. You can add those to this PR or open a new one.

2. Should the complete scope by default exclude canceled orders unless explicitly called for complete_including_canceled, the way that Product#variants by default excludes the master variant unless and we specify variants_including_master, or how various scopes exclude deleted objects unless we use with_deleted scopes?

No, complete just means that the order has, at some point in the past, been completed. In the core team, we were discussing we might be missing a name for a scope that is completed.not_canceled, but we did not come up with one.

[brchristian]

@mamhoff That all sounds good to me. I’ve closed the open issue at #2099 and am including here two additional commits: one to add the canceled and not_canceled scopes, and another to refactor the first commit in this branch to use that new syntax.

[brchristian]

@mamhoff I think I’ve addressed your notes here, so let me know if there’s anything else I can add to this!

[mamhoff]

Would you mind adding a simple request spec to document this behaviour change? Otherwise, this would be good with me.

[mamhoff]

@brchristian any news on this? Will you be able to add a request spec?

[brchristian]

@mamhoff Yes, happy to add a spec! Will try to get to that before the end of this week.

[brchristian]

@mamhoff Actually, just added the specs now. That should take care of it. Let me know if you want anything further and I’m happy to add it; otherwise I think we are probably good to merge.

[mamhoff]

I actually do. I think the model-level changes (including the spec, which is perfect) should go into one commit, and then we should have another commit that changes the controller to use the new scope.

Sorry to be so picky, but we'll all be thankful when we go back to the history of this. :)

[brchristian]

@mamhoff No worries, that sounds great. I’ve done a reorganization of those commits as you suggest, and have thrown in a rebase onto the latest master to boot.

[brchristian]

@mamhoff We good to go with this one?

[brchristian]

Hi @mamhoff just wanted to ping and see if we are good to go, would it be possible to merge this? We have been adding this in our app through a decorator and it’d be nice to remove that and simply be able to use the latest master.

I’ve just rebased again if that’s helpful!

[mamhoff]

@brchristian thanks for following up! Yes, in my opinion this is good to go, but needs another pair of eyes from the core team.

[tvdeyen]

Makes sense. Thanks

[brchristian]

Hi @tvdeyen are we good to merge this? Lmk if there is anything more we need and I am happy to hop on it. Cheers!