Add control parameters to analytics dashboard by Damonamajor · Pull Request #145 · ccao-data/homeval

Damonamajor · 2025-10-31T17:46:35Z

This adds 5 params to the homeval report to help produce more refined queries.
They are implemented both in the initial download and caching stage as well as the report.

start_date: initial date of data or a defined date.
excluded_dates: List of dates which can be removed from the query.
end_date: date that default to today unless otherwise specified.
refresh_today: TRUE FALSE indicator whether to query todays data.
ip_addresses_to_filter: List of IPs which can be removed from the query.

There is also a lintr styler circular error again which I just added #nolint to.

Damonamajor · 2025-11-03T18:13:37Z

+  dplyr::filter(
+    # End date filter
+    if (!is.null(params$end_date) &
+      nzchar(params$end_date)) { # nolint


This was giving a circular lint and styler error.

jeancochrane

This is trending in the right direction, but some of the code is a bit confusing and I'm not sure if it's working as intended. Let's get the intended behavior sorted out and then we can iron out the details.

jeancochrane · 2025-11-03T21:55:37Z

+    )
+  ),
+  if (is.null(params$refresh_today) ||
+    isTRUE(params$refresh_today)) { # nolint


[Nitpick, required] Currently, this behavior makes it so that refresh_today controls whether we refresh the end date, regardless of whether that end date is the current day. Can we either adjust this behavior or change the name of the param? Regardless of what we choose, let's also make sure we add a comment above the param in the params section of the doc config so that the behavior is documented.

Our two options:

Keep the current behavior and rename the param to refresh_end_date

Keep the current param name and refactor the behavior to add an additional check such that we only apply the filter if the end date is today

I don't feel strongly about which approach to choose, so I leave it up to you!

I have it so that it only uses the feature if the date is today's date, so that should help fix this behavior.

jeancochrane · 2025-11-03T22:00:20Z

+    # Start date filter
+    if (!is.null(params$start_date) &
+      nzchar(params$start_date)) { # nolint
+      date >= as.Date(params$start_date)
+    } else {
+      TRUE
+    },
+
+    # Excluded dates filter
+    if (!is.null(params$excluded_dates) &
+      length(params$excluded_dates) > 0) { # nolint
+      !(date %in% as.Date(params$excluded_dates))
+    } else {
+      TRUE
+    }


[Suggestion, optional] It strikes me that we're adding a lot of extra logic to this filter() call to account for the possibility that these parameters are missing/empty, when the convert_params block has already defined fallback values for the missing/empty case. Why not update the convert_params block to process excluded_dates, have emtpy_dates fall back to an empty vector, and then simplify this filter so that it incorporates the fallback values? Then the filter would look something like:

filter( date >= start_date, date <= end_date, !(date %in% excluded_dates) )

yup, I moved all the factoring up the the start to make everything work together. Good suggestion.

jeancochrane · 2025-11-03T22:04:28Z

+  setdiff(
+    all_dates[
+      (all_dates != end_date) |
+        (is.null(params$refresh_today) || isTRUE(params$refresh_today))


[Question, blocking] Am I right that this filter is duplicative with the logic on line 184? It seems to lead to duplicate end dates when I test it. Here's start_date == "2025-09-04" end_date == "2025-09-10" and refresh_today is null, note the two instances of 2025-09-10:

> dates_to_fetch [1] "2025-09-04" "2025-09-05" "2025-09-06" "2025-09-07" "2025-09-08" "2025-09-09" "2025-09-10" "2025-09-10"

jeancochrane · 2025-11-03T22:04:53Z

  message("Target file: ", fs::path_abs(out_file))

-  if (fs::file_exists(out_file) && d != today_local) {
+  if (fs::file_exists(out_file) & d != end_date) {


[Question, blocking] I'm finding it a bit confusing to wrap my head around how the filters in dates_to_fetch interact with this end_date filter. As far as I can tell, here's how it's currently working:

dates_to_fetch will only include end_date if refresh_today is null or true

If end_date is present in dates_to_fetch, it will always get refreshed, regardless of whether it is cached

Am I understanding the logic correctly? If so, I have two concerns about it:

If end_date is not already in the logfile cache and refresh_today is false, we will not query data for end_date.

Perhaps this is intentional -- I could see an argument that it would be more intuitive for refresh_today to prevent us from querying data for end_date if it doesn't exist -- but it feels like an extension of the parameter's behavior outside the scope that I was imagining for it. I was expecting us to use refresh_today == false to fall back to cached data for the current date if we already have it, but if we don't have that cached data in the first place, then I don't think it makes sense for refresh_today to skip it.

dates_to_fetch does not fully own the logic for determining which dates of logs to fetch, and if a code maintainer wants to alter the way it is defined, they have to remember that there is an interaction with end_date defined down here in the fetching logic itself.

I think there are a couple of ways we could resolve concern 2 to make the code easier to maintain, but the approach we choose will depend on the direction we want to take with the behavior described in concern 1. What do you think?

Co-authored-by: Jean Cochrane <jeancochrane@users.noreply.github.com>

Damonamajor · 2025-11-05T22:15:34Z

Thanks for the feedback. There is still the nolint included in the file, but I think cleaning the params up the script improves and simplified it a lot. I moved the refresh_today, to not query the cached data for today if the param does not equal false. That means that NULL TRUE or a random string should still query it.

Damonamajor · 2025-11-05T22:27:25Z

Something weird popped up, let me take a look.

Damonamajor · 2025-11-06T14:51:21Z

  message("Target file: ", fs::path_abs(out_file))

-  if (fs::file_exists(out_file) && d != today_local) {
-    message("Skipping ", format(d), " (already cached).")


This doesn't seem necessary.

jeancochrane

Awesome job here! I think the final code is nice and simple, and the filters worked great in every combination I tested.

Damonamajor added 3 commits October 31, 2025 17:24

add params for query and data controls

ddd75bb

lintr

a5fb9f7

rename param

76fcff9

Damonamajor linked an issue Oct 31, 2025 that may be closed by this pull request

Add control parameters to analytics dashboard #144

Closed

move params to download of parquet files

aadf872

Damonamajor marked this pull request as ready for review October 31, 2025 18:34

Damonamajor and others added 3 commits October 31, 2025 18:35

lintr

ff718d0

use old logic for end date

4860ee8

Update analytics_dashboard.qmd

fb6031a

Damonamajor requested a review from jeancochrane October 31, 2025 20:30

Damonamajor commented Nov 3, 2025

View reviewed changes

Comment thread reports/analytics/analytics_dashboard.qmd

Damonamajor commented Nov 3, 2025

View reviewed changes

Comment thread reports/analytics/analytics_dashboard.qmd

Damonamajor commented Nov 3, 2025

View reviewed changes

Comment thread reports/analytics/analytics_dashboard.qmd

jeancochrane requested changes Nov 3, 2025

View reviewed changes

Damonamajor and others added 6 commits November 3, 2025 18:05

Update reports/analytics/analytics_dashboard.qmd

8c2ca4c

Co-authored-by: Jean Cochrane <jeancochrane@users.noreply.github.com>

Update reports/analytics/analytics_dashboard.qmd

4772dc8

Co-authored-by: Jean Cochrane <jeancochrane@users.noreply.github.com>

Update reports/analytics/analytics_dashboard.qmd

ae1585c

Co-authored-by: Jean Cochrane <jeancochrane@users.noreply.github.com>

1st round of Jean edits

1faf58e

update query param for refresh_today

410f0a6

fix nolint

d190233

Damonamajor requested a review from jeancochrane November 5, 2025 22:15

Damonamajor self-assigned this Nov 5, 2025

re-include today local

3e343e1

Damonamajor added 4 commits November 5, 2025 22:35

lintr fix

e17b98b

remove params

a6dad34

remove today from cache structure

75093ea

lintr

c1ffdfa

Remove today from cached dates if not FALSE

4fbde55

Damonamajor commented Nov 6, 2025

View reviewed changes

jeancochrane approved these changes Nov 6, 2025

View reviewed changes

Damonamajor merged commit e35f9ca into main Nov 6, 2025
1 check passed

Damonamajor deleted the 144-add-control-parameters-to-analytics-dashboard branch November 6, 2025 22:39

Conversation

Damonamajor commented Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jeancochrane left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Damonamajor commented Nov 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Damonamajor commented Nov 5, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeancochrane left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Damonamajor commented Oct 31, 2025 •

edited

Loading

Damonamajor commented Nov 5, 2025 •

edited

Loading

jeancochrane left a comment •

edited

Loading