Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add case to shift_back function to handle day periods #2628

Closed
wants to merge 3 commits into from
Closed

Add case to shift_back function to handle day periods #2628

wants to merge 3 commits into from

Conversation

djanda97
Copy link
Contributor

Changes

Fixes #1650
This PR adds a case to the shift_back function in lib/plausible/stats/query.ex to handle day periods.

Tests

  • Automated tests have been added

Changelog

  • This PR does not make a user-facing change

Documentation

  • This change does not need a documentation update

Dark mode

  • This PR does not change the UI

@bundlemon
Copy link

bundlemon bot commented Jan 28, 2023 •
edited

BundleMon

Unchanged files (7)
Status Path Size Limits
✅ static/css/app.css
 492.34KB -
✅ static/js/dashboard.js
 298.05KB -
✅ static/js/app.js
 12.13KB -
✅ static/js/embed.host.js
 5.58KB -
✅ static/js/embed.content.js
 5.06KB -
✅ tracker/js/plausible.js
 748B -
✅ static/js/applyTheme.js
 314B -

No change in files bundle size

Final result: ✅

View report in BundleMon website ➡️

Current branch size history | Target branch size history

@djanda97
Copy link
Contributor Author

djanda97 commented Feb 4, 2023

Since the shift_back function handles month and year periods similarly, I tried following this format and adding another case for the function with the relevant parts swapped out for the day period. I haven't added an automated test case for these changes yet, but I wanted to open this PR to check if I'm on the right track. I would appreciate any feedback 😄

@djanda97 djanda97 marked this pull request as ready for review February 4, 2023 21:56
@ukutaht
Copy link
Contributor

ukutaht commented Feb 6, 2023

I haven't added an automated test case for these changes yet, but I wanted to open this PR to check if I'm on the right track. I would appreciate any feedback 😄

Yes I think you're on the right track :) We'd need the automated test cases in order to verify this change properly.

@ukutaht ukutaht mentioned this pull request Feb 28, 2023
Merged
4 tasks
@vinibrsl
Copy link
Contributor

vinibrsl commented Mar 30, 2023 •
edited

Hey 👋!

First of all, thank you for your contribution, David!

We're building a comparisons feature that uses the shift_back functions extensively. In the context of this work we've refactored this function completely using a new strategy. The shift_back function itself does not exist anymore and it is equivalent to this clause of the do_compare function:

analytics/lib/plausible/stats/comparisons.ex

Lines 67 to 78 in 9a4c4f4

defp do_compare(source_query, "previous_period", opts) do
now = Keyword.fetch!(opts, :now)
last = earliest(source_query.date_range.last, now)
diff_in_days = Date.diff(source_query.date_range.first, last) - 1
new_first = Date.add(source_query.date_range.first, diff_in_days)
new_last = Date.add(last, diff_in_days)
range = Date.range(new_first, new_last)
{:ok, %Stats.Query{source_query | date_range: range}}
end

The code itself is simpler because instead of having a case for each requested period, we limit the end date to the current day.

The bug your pull request is trying to solve is still there after the refactor, and I'm not sure this pull request actually fixes it. The shift back function was in fact shifting back 1 day for day periods, which is not entirely correct, as this does not take the time into account.

If you're still interested in tackling this issue, I'd be happy to give you more context and to help as needed. I'd suggest you to start over, as this function has been entirely refactored, and working on the time precison from a clean slate.

@djanda97
Copy link
Contributor Author

djanda97 commented Apr 1, 2023

Hi @vinibrsl! It has been a while since I've worked on this issue plus there wasn't much code that was included in this PR, so I agree that it would be best to start over. I'm still interested in working on #1650, so I'll take a look through analytics/lib/plausible/stats/comparisons.ex and its associated tests. After taking a closer look, I'll post here with any questions that I have! 😄

@djanda97
Copy link
Contributor Author

Hi @vinibrsl, I haven't found the time to work on updating this PR.

@djanda97 djanda97 closed this Jun 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Statistics relative to previous period are misleading
3 participants
@djanda97 @ukutaht @vinibrsl