Add events view #4093
Add events view #4093
Conversation
|
@dorner I was going to ask for this! Obviously needing the tests to pass, but if we can I'd like to see something like a date on the entries . Is it possible for us to say whether it is new or edit? Also - we need to make the column headers more relatable -- because to them, "Event" is likely to sound like "the thing I did" -- which to them will be more like what the "resource" is (I'm assuming resource is the itemizable). What would you think of something like "internal id" and "Refers to".? |
|
Tried it on a fresh setup on local. Did not turn on the events. 1/ Was able to get at it -- I had assumed this would be behind the events flag 2/ Got the following: 
I don't know if that's a problem with the seed, or what, but I'll need that to work to be able to kick the tires enough to give a better opinion. |
There was a problem hiding this comment.
Hey @dorner Thank you for getting this far. I'm really hoping we can include this in when we go to early adopters. I've got an idea or two to make it more approachable for our key users.
Alas, it feels like maybe this one was a bit of a rush to get in before your week-end deadline -- it's not quite at your usual standard for getting the i's dotted and the t's crossed. I'm afraid there are some problems with the tests, with the lint, and with being able to run it on a fresh setup on my local.
|
It was definitely rushed, you are 100% correct. Didn't even have time to see if the tests and lint finished before logging off. No problem at all waiting for fixes. |
|
Btw I didn't put it behind the flag because the events are still happening, they're just not being used. But if you think it'd be confusing, I can put it behind the flag. |
|
I think we'll need to rethink the terminology for in front of the users. A new user is not going to have any idea of what "Event History" means. Maybe just "History"? I'd be ok with putting it in front of the early adopters and explaining what it is to each of them as is, so if you could put it in behind the flag that'd be great. |
|
At this point, I see this as mainly a helping-with-diagnoses tool, but it may end up being something someone ends up relying on. |
|
@cielf I've made the suggested changes. The crash was due to SnapshotEvents having a different format than the rest (not sure why I didn't come across it before). Should still have a fast-follow where I add more tests and filters. |
|
I didn't add the event flag because that's not merged yet :) I can put it in as soon as it is. |
|
Ok...just dipping my toe in the water of kicking the tires. Here's what I did, what I expected (given the expected use as a troubleshooting tool) and what I see: I started with a fresh seeded setup, and then I went and edited a purchase, to add an item to it (since the seed has none). |
|
As for the event flag - I think we should have it before it goes live, but it's actually a bit convenient to have it not on at this point. |
|
Good point - the history date doesn't differentiate between creation and update. I'll put something in there for that. |
dorner
force-pushed
the
event-history
branch
from
b3779a5 to
abf232a
Compare
|
Hmm... actually that's a lot harder than it sounds. I think leaving it as is is probably better (i.e. a "Distribution" event could mean creation or update). I'll check the case you mentioned. |
|
If it doesn't give the whole story, we might want to keep it to ourselves, though. In my mind its purpose was for walking through what happened step-by-step. Almost a log. Let's talk about it Sunday. |
|
I've added type and storage location filters: Unfortunately I'm reeeeally behind on tests (i.e. I don't have any for any of this). Hoping to get some of them in on Monday. |
|
Oh I also added the event flag check. |
|
@cielf added the item and eventable filters, and added tests. |
|
I kinda love the little funnel gizmo now that I tried it out. Nice touch. The one thing I've got some qualms about is the ordering. It is the opposite of the default order on everything else, so there may be confusion. This is not me saying to change it, because when we are using it for the purpose of "ok, then what happened... then what happened next..." the chronological order makes good sense and will be easier to use. It might only be tripping my "what's going on here" wires because I'm looking at it on the seed, which is all from today. If you have real data, that has been collected over time, it'll be clearer? I think we can send it to the early adopters like this, but we should consider if there is a reasonable cue we can provide - just because it's the opposite of the order everywhere else. (Maybe even just say History (in chronological order) as the title?) |
|
Alas, the Filter by storage location isn't filtering completely. I tried it with Pawnee Main Bank(Office), and I'm still seeing the snapshot for Bulk Storage Location. Everything else looks reasonable in that case. |
|
Snapshots aren't per-storage-location, they're global. So by design the filter includes all snapshots. |
But they say that they belong to a storage location in the view.... |
|
They have multiple storage locations. If you scroll down you'll see both. There isn't a great way to split that up in the view. |
|
Hmmm. How often are we doing snapshots? Just to start the thing off, really? |
|
For now. We probably will introduce more in the future for performance purposes. |
|
Ok. I think that might end up being confusing to the users, but I'm willing to early adopt this as it is and kick that particular can down the road until we see if people end up using this for anything but troubleshooting. I'd like @awwaiid to take a look at this from a technical pov. (Though I'm pretty sure it will pass with flying colours once you clear the failing tests/lint.) |
|
@cielf I added a separate background color for snapshot events which should at least make it more obvious that the two rows are grouped together. Lint and tests should be fixed. |
|
(nods) I can foresee a future, if this is useful to the banks for something other than this troubleshooting, where we hide the snapshots by default, and have an "include system snapshots" checkbox, but that's good for now. |
|
@awwaiid Can you do a quick pass on this from a technical pov. LGTM from a functional pov. Thank you. |
|
@dorner Ok. This is late-breaking, but we've got the user information on the event, yes? That could actually be handy when we are talking through troubleshooting with the banks. But I'm totally willing to have it be a fast follow-up in the interest of getting this in. |
|
I've replaced Internal ID with user name. The ID isn't helpful to anyone but us. If we really need it, I added it as an HTML tooltip on the "event type" column. |
|
(nods) That sounds reasonable. Thank you. |
|
I fixed the one easy spec error, but I'm not sure on the other one -- looks like events without users. |
|
Should be fixed now I hope! |
|
@dorner: Your PR |
Fixes #4079.
This adds a simple events view for org admins:
Missing some features:
We can do that as a fast follow.