User chart #1667
User chart #1667
Conversation
The styles for the charts on /about/charts.html were defined in a <style> block in the simplate. Now we want to use those styles on a chart on another page (#1514), so let's move them into the global stylesheet.
Next step is to make it a proper library.
The two that aren't are cumulative and nwithdrawals, which need some math on them. These are the existing charts, mind you. Once these are taken care of I'll look at implementing the per-user chart.
I decided to move the calculation to the server-side rather than complicating the chart API on the client-side. This introduces a new endpoint, /about/charts.json.
I did this as a subselect in the charts.json query.
I was using the wrong column from paydays.
The way we do the X axis in the chart kind of requires this. I guess it also makes it easier to compare across users and with the overall system.
|
The feature is implemented and ready for testing, though the code doesn't pass JSHint. |
The "Weekly" is redundant (on the about/charts page it is to distinguish from cumulative gifts), especially next to "Number of Patrons."
|
IRC re: JSHint |
|
Ready for review and merge! Waddya say, @wyze? For old time's sake? It's user-facing! :-) |
I had started with these charts down there, but when I moved them up to the right column above I forgot to remove them here.
| if not receipts: | ||
| charts = [] | ||
| else: | ||
| charts = receipts + paydays[len(receipts):] |
There was a problem hiding this comment.
I see what you are trying to do here but what happens when user for one week has transfers and for the next all tips are cleared? I believe we need to convert paydays to dict keyed on date and update it by receipts also keyed on date.
There was a problem hiding this comment.
And additionally this will break with each transfer done outside of payday (like we discussed in issue #54 where I have voiced the concern not knowing if it is actually true). I think we should map each transfer to some payday here so that the whole week gets covered but the chart is still by payday.
There was a problem hiding this comment.
Good eye. Addressed in d02ed22, where I switch to aggregating based on the actual payday timestamps, not just the date. This made testing a lot easier, too. I added tests for both the "bad week" case you mention, as well as the case of an out-of-band transfer (cf. #54 (comment) ff.).
This addresses several issues. First of all, we had been aggregating based on date, which is safe in production (for now?), but makes testing much more difficult. Now we aggregate based on the actual timestamps of the paydays in question. We do so in a way that isn't too taxing on the db. The transfers query takes about 50ms for my user from a recent backup. This fixes a bug where a user who received tips for a few weeks and then had a week where they didn't get any tips would ... I don't know, but it was a bug. We now have a test for that case, as well as the case where a transfer happens in between paydays.
Conflicts: www/about/charts.html.spt
When the flags go right then they overlap any chart to the right. We had a fix in #1677, but it was kinda a hacky solution and it didn't adapt well to changing chart widths and I also clobbered it when merging master out to the #1667 branch. So here's a different fix for the same problem: flip the flags.
This addresses #1514.