-
-
Notifications
You must be signed in to change notification settings - Fork 2k
Extend stats timeline to current day #3895
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
The stats timeline stopped at the last click. This change ensures users see stats up to today. Now pads $list_of_days with zeros from the last click to today in yourls_build_list_of_days().
Looks like a fine approach, though you have removed whitespace that is part of our (under-documented?) format standards. I'd appreciate it if you unremoved those spaces :) @LeoColomb Do you think we can ship this + #3893 as 1.10.1 soon? Broken plugin deactivation in particular is going to frustrate people if it sticks around too long in the stable release. |
YOURLS style prefers no space before parentheses in control structures. Fixed spacing in yourls_build_list_of_days() (e.g., `if (` to `if(`) for consistency, as requested in PR review.
The white space removal was accidental, only noticed it after I'd already pushed the commit. Fixed it now :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (but not tested).
@dgw Agreed 🚀
Thankfully it's easy enough to copy-paste I don't have a super active instance, so the highest-click-count example I can test ain't much. It's enough to confirm that this patch does what it says, though: Compare to the old line chart, before I updated Note that the "last 7 days" and "last 30 days" options disappeared after this patch, because there is no data within those time periods. That's probably appropriate. Why invite the user to view empty data? What worries me a little more is that the click total I get by summing the points in the updated line graph view is only about half the actual total reported in the table to the right. (Probably that is from |
Can you check if the total in the right table corresponds to the number of:
As the two might be different, it's eventually all fine. |
The total click count is correct. As I said before, it's probably YOURLS/includes/functions-infos.php Line 296 in b99bbfd
Changing that (to e.g. sum the values in each "bucket" instead of dropping the in-betweens) could be an interesting thought, but out of scope for this PR 😸 |
Oh right, apologies (again!), I read your comment too quick. Right, so ready to merge? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, so ready to merge?
Yes
Fixes issue #3695 - Traffic statistics is incorrect in case of no recent activity. The stats timeline stopped at the last click. This change ensures users see stats up to today. Now pads $list_of_days with zeros from the last click to today in
yourls_build_list_of_days().
Tested with multiple URLs to confirm all timeframes display correctly.