Add a user network plot #17
Conversation
|
This PR contains the logging changes from #16, which has been merged, so this conflicts. Can you rebase? Otherwise this looks pretty good, I'm trying it out now! |
This takes out the code for creating plots from the report template as it may also be useful in other contexts. In addition to that, the time limit is removed. I would expect the data to either be prefiltered for the desired timespan or the initial fetch to have an appropriate `since` parameter. Also, some of the message type filtering was changed because I encountered some bugs in the process.
|
I have rebased the branch. I wonder, why the commit from #16 is different now. Normally there shouldn't be any conflicts if the commit is identical. I mean, that's what you ensure by rebasing. |
|
I rebased #16 on merge, and rebase changes the SHAsum - so Git probably doesn't realise they're the same anymore. |
|
|
||
| ### Daily Activity & trend (last 30 days) |
There was a problem hiding this comment.
Any particular reason why you're dropping the 30 day filter here?
We pass rooms rather than rooms$events to the report, so it's not trivial to filter the event list before calling the report. Perhaps we should allow a filter YAML parameter instead?
There was a problem hiding this comment.
I thought that the data would either be prefiltered as desired or only reach back to the time. But going back to it, I think the functions for plotting should accept the time filter parameter with a default value.
There was a problem hiding this comment.
Yeah, that makes sense. I was seeing some weirdness in my get_rooms calls after merging #16 that I am not seeing with these patches, so I think I'm going to merge this in the next hour or so (to keep HEAD working cleanly). We can roll-forward on adding time filters to the plots.
There was a problem hiding this comment.
I updated the branch reintroducing the time limit. But as it applies to all plots, I added it to the introducing paragraphs. I think that this makes more sense, but we can also go back to the initial visuals.
There was a problem hiding this comment.
Oh, thanks! I wasn't trying to make you rush, I would have been happy to merge as is and then fix this later - but this is great. I agree we probably need to revisit the report itself - the new member calls mean we should probably add some of that data too.
In addition, an optional parameter allows for tweaking the desired timespan.
|
Merged, thanks @johrpan! |
This PR does the following things: