Site-kit-wp: Properly handle date range dropdown values with props instead of filters

[felixarntz]

The date range filter value is applied via a filter to the entirety of requests in a batch request (in data.combinedGet()). As such, it is impossible to make requests with individual date ranges. Furthermore using a filter fired in the low-level API request function violates React's development principles by which we should pass such a selected value as a prop to the components that need to use it.

Note that despite #465, the API layer should be decoupled from that, and every request should be able to have an individual date range value. In our case, it will for now be the same everywhere, but this shouldn't be enforced by how the API works.

Secondary problems with the current JS implementation that need to be fixed:

• A translation string is used to create a slug. This is a bad practice which will definitely lead to issues as soon as the plugin is translated. It works for English (where the untranslated Last 28 days becomes last-28-days), but in another language the slug would become something completely different, which the server would be unaware of (e.g. in German Letzte 28 Tage would become letzte-28-tage --> server does not have a switch case for that). Because of that, slugs must be decoupled from the user-facing dropdown options.
• There's no point of having individual translation strings Last 28 days, Last 14 days, etc. Instead, there should only be a single translation string Last %d days, used in combination with sprintf() to fill in the number. This makes it less work to translate, and it even allows for more flexibility (e.g. supporting a different number of days).

The implementation of this issue should only start after #406 is merged. However, preparing the implementation brief etc. can already happen before.

---

_Do not alter or remove anything below. The following sections will be managed by moderators only._

Acceptance criteria

• The date range for the current screen should be controlled on a per-screen level.
  
  
  
  • The date range dropdown React component should receive a function for updating the date range value as prop.
  
  
  • The date range value controlled by the screen should be passed as props to any "widgets" (white boxes with content) on the page so that they can use it.
  
  
  • The components that require requests to run (via withData) should pass the dateRange from props in their requests.
  
  
  • The date range filter in data.combinedGet() and data.combinedGetFromCache() should be removed. Date range should just be another parameter (thus not be specifically handled at all).
  
  
• The date range dropdown component should be able to receive available options as props (with value and label for each one). There should be a default list for when they're not provided which should match the currently available dropdown options.
• Translation strings for Last x days should be composed with sprintf() so that there is just a single translation string for these (e.g. sprintf( _n( 'Yesterday', 'Last %d days', 28, 'google-site-kit' ), 28 )).

Implementation Brief

_Revisit when Data layer and API/request approach is entirely overhauled. For now, it's best to close this issue as a won't-fix._

---

See WIP / PoC https://github.com/google/site-kit-wp/compare/poc/propify-date-range-selector

• Remove all hooks from DateRangeSelector
• Update default options to use last-{n}-days values regardless of translation (keep 7, 14, 28, and 90 day choices)
• Make options customizable via prop
• Require an onChange function prop, which accepts the new value in an object when changed

data.js

• Remove filter in data.combinedGet()
  https://github.com/google/site-kit-wp/blob/b42304c5e577eb13fa5934bd9c02f2a0cba1e44b/assets/js/components/data.js#L143-L148
  https://github.com/google/site-kit-wp/blob/b42304c5e577eb13fa5934bd9c02f2a0cba1e44b/assets/js/components/data.js#L160
  Date range will automatically be built into the cache key as part of #406
• Remove in data.combinedGetFromCache()
  https://github.com/google/site-kit-wp/blob/b42304c5e577eb13fa5934bd9c02f2a0cba1e44b/assets/js/components/data.js#L75-L80
  https://github.com/google/site-kit-wp/blob/b42304c5e577eb13fa5934bd9c02f2a0cba1e44b/assets/js/components/data.js#L91
  Date range will automatically be built into the cache key as part of #406

Modules

• Remove filters from analytics and search-console modules
  
  
  
  • https://github.com/google/site-kit-wp/blob/b42304c5e577eb13fa5934bd9c02f2a0cba1e44b/assets/js/modules/analytics/index.js#L137-L139
  
  
  • https://github.com/google/site-kit-wp/blob/b42304c5e577eb13fa5934bd9c02f2a0cba1e44b/assets/js/modules/search-console/index.js#L109-L111
  
  

Components

• DashboardModuleHeader change timePeriod prop to dateRange which should accept the current value.

Utils

• Remove getDateRangeFrom() utility (based filter). Consuming code should be updated to reference value from prop.
  https://github.com/google/site-kit-wp/blob/b42304c5e577eb13fa5934bd9c02f2a0cba1e44b/assets/js/util/index.js#L906-L913

Providing and Consuming Date Range

Due to the current non-parent-child structure for data-flow using filters to share the state of the current date range, while moving to a prop-based implementation, it makes sense to leverage React Context. This allows us to minimize the number of intermediate components that would require accepting and passing props through to children (prop drilling).

Implementation may look similar to this:

• Top-level Dashboard... components will import a new DateRangeContext
• Top-level Dashboard... components will add dateRange to their state, and define a setDateRange method, which should function similar to this
  js ( newDateRange ) => this.setState( { dateRange: newDateRange } )
• Top-level Dashboard... components will wrap their child components with the <DateRangeContext.Provider value={{ dateRange: this.state.dateRange, setDateRange: this.setDateRange }}
• Then DateRangeSelector should utilise the useContext hook (eg useContext(DateRangeContext)) to get the value and use the update function to modify the data stored in context.
  > This pattern is outlined in the React documentation here:
  https://reactjs.org/docs/context.html#updating-context-from-a-nested-component

Changelog entry

[tofumatt]

Translation strings for Last x days should be composed with sprintf() so that there is just a single translation string for these (e.g. sprintf( __( 'Last %d days', 'google-site-kit' ), 28 )).

Because these strings deal with numbers of different sizes I don't think we should have a single translation string as part of the acceptance criteria.

If the day number was 1 we would likely want a different string entirely (eg. __( 'Since yesterday', 'google-site-kit' ) or maybe __( 'In the past day', 'google-site-kit' )), so we likely want to use wp.i18n._n() here instead of a single string. An "in the past day" option could prove a useful future addition, as I could imagine that being useful during an intense traffic spike to a site.

I notice the lowest number is currently "7 days"; even so we should still be using _n() for languages with more than one plural form (ex. Czech, Polish).

So I think the acceptance criteria shouldn't specify a single translation for that string. It should specify using _n()

[aaemnnosttv]

+1 to what Matt said. I've used _n() in my poc linked above, although I used Last 24 hours.

Updated the IB but there is still more to cover.

• Affected components that use DateRangeSelector internally, such as DashboardModuleHeader or PageHeader
• How and where the current date range should be stored and how it is provided. In the future, I imagine this will live in one of the core/ data stores, but for now perhaps it belongs in googlesitekit.admin.dateRange ? Also, should this value persist between different screens (use screen specific keys) or should this be a single value ? I would think the chosen value would be global (at least for the screens we have now). Some screens should be able to show a different date range if necessary.
• Changes to components that use withData
• Do for use in data object/parameters, should we keep a last-x-days value format or should we standardize on a simple numeric value? E.g. lastDays: x

[felixarntz]

@aaemnnosttv IB looks great for a start! Some thoughts:

Update getDateRangeFrom() to get the current date range from the screen context

Can you clarify how you mean that? Wouldn't we pass the dateRange to all "widget components" as a prop, so they can pass it via withData? One goal here is to decouple the date range so that it can also be passed per component (potentially with different values).

How and where the current date range should be stored and how it is provided. In the future, I imagine this will live in one of the core/ data stores, but for now perhaps it belongs in googlesitekit.admin.dateRange ? Also, should this value persist between different screens (use screen specific keys) or should this be a single value ?

I think we don't need to focus on any persistence between requests for now. Potentially it should live in a core/ data store later, but it might also be sufficient to just handle it in the individual ...App component's state (that's what I'd suggest for now at least):

• Have dateRange as state var in every relevant ...App component, and allow it to be updated via DateRangeSelector and the new update callback prop.
• Pass the dateRange state value as prop to the respective ...Modules component.
• From there, pass it as prop into every module that is filtered in (via withFilters).

Do for use in data object/parameters, should we keep a last-x-days value format or should we standardize on a simple numeric value? E.g. lastDays: x

I think we should keep the option values as last-x-days, not just the number, in order to potentially allow last-x-hours, next-x-days or anything like that. I'd say the options in the dropdown should already match that, and not just be the integer.

@tofumatt @aaemnnosttv Completely agree on the translation string. I thought about that before, but thought we'd probably never have "1 day". But your argumentation makes sense, it's a bit more future-proof. I'd prefer to go with Yesterday for the singular translation though, since that is consistent with the others, where we're talking about days, not hours.

[aaemnnosttv]

Update getDateRangeFrom() to get the current date range from the screen context

Can you clarify how you mean that? Wouldn't we pass the dateRange to all "widget components" as a prop, so they can pass it via withData? One goal here is to decouple the date range so that it can also be passed per component (potentially with different values).

This utility should probably just go away then since there would no longer be a global value to reference. I was thinking this would use the same top-level value for the page, but it's probably unnecessary now.

I think we don't need to focus on any persistence between requests for now.

My concern here would be that without persistence, the range would be reset on every page load, which would be confusing. I think it's probably fine for it to live in the ...App component states but the initial state should probably be loaded from something persistent.

I think we should keep the option values as last-x-days, not just the number, in order to potentially allow last-x-hours, next-x-days or anything like that.

👍

[tofumatt]

Reading the IB here, my thought is that this is a good opportunity to start moving data into wp.data rather than into React's Context API.

Since the plan going forward is to move to using state management, architecting the date range selector to use wp.data would be more idiomatic of the direction we want to move the codebase in. The issue is that it's more work than adding a bit of React Context, but were I building this sort of component from scratch my first thought would be "global state" and not "React Context".

This would avoid this part of the IB:

Top-level Dashboard... components will add dateRange to their state, and define a setDateRange method, which should function similar to this [...]

Instead of managing a dateRange in their own internal states and a setDateRange method, we can use selectors to get the data by components that need the data and pass it as props. For setDataRange we would use a dispatch function to update the store.

---

I would also recommend _storing_ the last-x-days data as something a bit more direct that won't require parsing the string to get at the data. I guess I'm not sure on how that API request is made, but assuming it's with an offset something like { offset: -604800 } (for 7 days ago, in seconds) might be nice. Would give us the ability to select an exact time in the future with something like { since: DateTime }. Unless the arguments to the API that fetches the data will take arguments like 7-days-ago 😄

[aaemnnosttv]

Just following up here that the selected date range value does not persist between page loads, and comes up as "Last 28 days" every time.

I would also recommend storing the last-x-days data as something a bit more direct that won't require parsing the string to get at the data.

I thought about this and agree that this particular format is not ideal in that it requires parsing. I like the idea of an offset in that the storage format would be consistent and more future-proof to allow for any date/time range. This would require some kind of conversion to human time range string function though. I suppose for now it would be probably fine to assume it's a number that's evenly divisible by days-in-seconds, but eventually we would probably want something more flexible/intelligent.

[felixarntz]

I thought about this and agree that this particular format is not ideal in that it requires parsing. I like the idea of an offset in that the storage format would be consistent and more future-proof to allow for any date/time range.

I think an offset alone unfortunately doesn't match some requirements here. It'd work fine for now, but a good approach for potential future requirements would be looking at the AdSense code that supports a lot more specific date ranges, some of which might also make sense to make available globally at some point. For example, sometimes AdSense is not looking for just the last x seconds, but something more complex (e.g. "every full day last month", or "every full day this month plus whatever has passed of today").

An alternative would be to store a numeric value alongside a string for the type of measurement (e.g. last-days, last-month, last-hours). But probably the only way to cover any potential scenario is to store exact start and end date, which however is a bit far fetched now.

[tofumatt]

Having chatted about this versus wp.data, in the interests of time we can use context for now and tackle moving to global state later. I updated the IB to favour using React Hooks to consume the context data instead of the HOC.

I still think the AC needs to be updated to include the _n() approach but I didn't think I should modify it. 😅

[felixarntz]

IB looks great ✅

@tofumatt Do you think it makes sense to work on #465 before to simplify the work needed on this one?

[tofumatt]

Yes, this should be considered blocked by #465. Taking care of that first means fewer things here to change. It's not _technically_ blocking, but that makes sense to me 👍🏻

[tofumatt]

I still think there's a fair bit of re-architecting here, so even after #465 is closed this will still be a medium-sized issue.

[tofumatt]

This is blocked by #465, so please move this out of blocked once that issue is resolved.

[felixarntz]

Moving this back into "To Do" because it's unblocked, but removing this milestone as it's too late for that. Let's keep it in the sprint though.

[tofumatt]

I'm still navigating the code required to update for this issue to move along. It's a lot harder to track down than I thought and it's very difficult to trace the code path of the rendered components here.

I'm also a bit concerned about implementing this without a global data store, as the approach outlined regarding state in the parent component being linked to context may require re-implementing the shared state <> context relationship in multiple components.

I'm still kinda unclear about how all this code works together so maybe I just need to keep digging, but for now I'm updating this to be a large issue, as right now it's quite the rabbit hole. 😓

[tofumatt]

After having been walked through the withData HOC a bit more and the nature of the rendering/component tree (that many things aren't inside the component tree but injected afterward), I'm still not clear on how to integrate React Context and prop-passing into this solution when maybe things are added with filters and not part of a component tree.

My first instinct upon understanding the withData HOC was to pass dateRange props to the component using withData but I don't find that to be clear either. At this point I'm pretty lost on this issue and I think need a primer on the data flow and logic flow of the main dashboard components. I'll leave myself assigned for now as I think it's important to understand the mechanics at play here, but it's possible taking this issue might not provide a lot of benefits until a more straightforward data flow is established. Or maybe I'm just lost on a key piece of info here and need to let the explanations I've received so far marinate 😅

[tofumatt]

After examining the nature of the codebase I don't think this is solvable as-is without a massive refactor—which I've been attempting in https://github.com/google/site-kit-wp/compare/chore/447-date-range-use-props?expand=1 but have been stumped by. The API requests are _very_ disconnected from the DateRangeSelector, which seems to have been added late and is added to the request in https://github.com/google/site-kit-wp/blob/93ca9a922f4091f6ddf25b970133c8639542f723/assets/js/components/data.js#L69)

Using context that would be available to withData and DateRangeSelector seems to require adding the context at the top of the page, which effectively forces each DateRangeSelector to share the value with the page.

Honestly, I'm struggling to even clearly trace things back to the relevant withData() calls that make these requests. My branch attempted to refactor the withData component—as right now its requests were made in the constructor, meaning updates to props would not be fired. Additionally, because of how doAction is required to clear caching the expected behaviour of using props here doesn't work as expected.

Overall I'm not sure this is worth approaching until all of the API/fetching/request layer is overhauled.

[tofumatt]

(I've unassigned myself from this issue—I think the existing IB are not sufficient but it's worth revisiting if we want to pursue this at all while the data layer has the kinds of shortcomings it has.)

I think it's best to leave this alone, I've already gone down quite a rabbit hole with it.

[aaemnnosttv]

Thanks for flagging this @tofumatt. I'm going to move this back to Blocked for now until we decide where this will go next.

[ThierryA]

Is this still blocked? If yes, could we added the dependency?

I put it in the Prioritization column as it doesn't have a priority and estimate.

[aaemnnosttv]

@ThierryA I believe the consensus was that it wasn't worth the effort to address this until we have a proper data store in place to handle the date range in a way that dependent components are updated when they need to be. @tofumatt may be able to elaborate a bit more as he was the last to work on this.

[ThierryA]

Ok, do we already have an issue for the data store? If yes, let's make this a dependency of it, if now, we can make it a dependency of the JS APIs refactoring epic.

[tofumatt]

I'll set this as a dependency of the JS API refactoring for now, as we don't have any issues specific enough to this that are appropriate, and filing it right now is probably a bit early (we have a bunch of issues in a Google Doc somewhere but we've been filing them in stages to make it easier to manage 😄).

[aaemnnosttv]

@felixarntz @tofumatt I think we can probably close this issue as the idea is correct but these need to be refactored to use the datastore rather than simply accept props since the value will come from the datastore anyways.

Interesting question though - which datastore will date range state belong to since it is used cross-module?

[felixarntz]

@aaemnnosttv Closing this sounds good to me. I think we can add the date range to core/user since it's mostly a user-based setting. Potentially we'll want to maintain it throughout sessions to stay the same too, but that should be per user and not for the entire site.

[aaemnnosttv]

@felixarntz Closing this in favor of #1529 and #1531