10000 Extend stats timeline to current day by ntindicator · Pull Request #3895 · YOURLS/YOURLS · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

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

Merged
merged 3 commits into from
Apr 24, 2025
Merged

Conversation

ntindicator
Copy link
Contributor

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.

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().
@dgw
Copy link
Member
dgw commented Apr 11, 2025

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.
@ntindicator
Copy link
Contributor Author

The white space removal was accidental, only noticed it after I'd already pushed the commit. Fixed it now :)

Copy link
Member
@LeoColomb LeoColomb left a 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 🚀

@dgw
Copy link
Member
dgw commented Apr 14, 2025

(but not tested)

Thankfully it's easy enough to copy-paste functions-infos.php over to my server to test it out.

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:

image

Compare to the old line chart, before I updated functions-infos.php from this PR branch:

image

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 yourls_array_granularity() and not a result of this patch. It's just more likely as the link ages to drop meaningful values from the larger set.)

@dgw dgw added this to the 1.10.1 milestone Apr 14, 2025
@LeoColomb
Copy link
Member

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.

Can you check if the total in the right table corresponds to the number of:

  1. Click count in the database (I expect so)?
  2. The number of lines in the database log table for the corresponding link?

As the two might be different, it's eventually all fine.

@dgw
Copy link
Member
dgw commented Apr 17, 2025

Can you check if the total in the right table corresponds to the number of:

The total click count is correct. As I said before, it's probably yourls_array_granularity() messing with things to get a "smoother" graph at the expense of accuracy, as it says in the doc comment:

* This make less accurate but less messy graphs when too much values.

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 😸

@LeoColomb
Copy link
Member

As I said before, it's probably yourls_array_granularity() messing with things to get a "smoother" graph at the expense of accuracy, as it says in the doc comment

Oh right, apologies (again!), I read your comment too quick.
I see now.

Right, so ready to merge?

Copy link
Member
@dgw dgw left a 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 :shipit:

@dgw dgw enabled auto-merge (squash) April 24, 2025 23:06
@dgw dgw merged commit 57e8c78 into YOURLS:master Apr 24, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants
0