google / site-kit-wp

Site Kit is a one-stop solution for WordPress users to use everything Google has to offer to make them successful on the web.
https://sitekit.withgoogle.com
Apache License 2.0
1.23k stars 286 forks source link

Add the “expirable items" infrastructure #8168

Closed techanvil closed 3 months ago

techanvil commented 8 months ago

Feature Description

Add the generic “expirable items" infrastructure that will be used to manage the "New" badges.

See "New" badges in the design doc.


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

Acceptance criteria

Implementation Brief

Note that the infrastructure should leverage existing APIs where possible, for example it may be viable to build it on top of the dismissed items infra in conjunction with the use of WordPress transients.

Adding selector and action to core/user store

Note: Take a reference of assets/js/googlesitekit/datastore/user/dismiss-items.js to create the store.

Test Coverage

QA Brief

Adding the expirable items

  1. Run following command in browser console.
    googlesitekit.data.dispatch('core/user').setExpirableItemTimers([
    {
        slug: 'foo',
        expiresInSeconds: 100,
    },
    {
        slug: 'bar',
        expiresInSeconds: 200,
    },
    ]);
  2. This should trigger a request in network tab of the browser to core/user/data/set-expirable-item-timers endpoint. Ensure that the request is successfully executed with response 200.

Fetching the expirable items

  1. Run following command in browser console.
googlesitekit.data.select('core/user').getExpirableItems();
  1. This will return the object like following. Note that we are preloading the expirable items, so this should not send a separate network request.
{
    "foo": 1715622594,
    "bar": 1715622694
}

Note that timestamp may be different which is object value like 1715622594.

Checking whether the expirable item is active

  1. Create an item which should expire after 10 seconds with the help of following command.
googlesitekit.data.dispatch('core/user').setExpirableItemTimers([
    {
        slug: 'shortSpan',
        expiresInSeconds: 10,
    },
]);
  1. Refresh the page after 15-20 seconds and run following command.
googlesitekit.data.select('core/user').isExpirableItemActive('shortSpan');

this should return the value false as the item must have expired.

Checking whether the expirable item is exists

  1. Run the following command in browser console.
googlesitekit.data.select('core/user').hasExpirableItem('foo');
  1. This should return true as we have created foo item above.

  2. Run another command in browser console.

googlesitekit.data.select('core/user').hasExpirableItem('baz');
  1. This should return false as we have never created baz as expirable item.

Changelog entry

techanvil commented 7 months ago

Note that I've moved this back to the Backlog pending the outcome of this discussion on the design doc regarding the shape of the API.

techanvil commented 7 months ago

Said discussion is now resolved and this issue is ready for AC.

eugene-manuilov commented 6 months ago

AC ✔️

techanvil commented 6 months ago

I've moved this back to AC pending an update once we've confirmed changes to the infrastructure underpinning the "New" badges, as discussed on Slack.

techanvil commented 6 months ago

Update: I've moved this to the Backlog instead to make it clearer which issues are waiting for a change or approval to the design doc (see this comment on the doc).

techanvil commented 6 months ago

The related design doc discussion has been resolved and this is ready for IB.

zutigrm commented 5 months ago

Unassigning myself as this is squad 2 related

techanvil commented 4 months ago

Hi @ankitrox! Thanks for drafting this IB, nice work so far. A few points:

  • [ ] The option will have the following data structure:
       array(
           'item-slug-1' => array(
               "expires" => 1700000000
           ),
           'item-slug-2' => array(
               "expires" => 1690000000
           ),
       );

Arguably, we don't need a nested array structure here, if we only have the one subkey expires. We could follow the example of the Dismissed_Items setting, i.e.:

array(
    'item-slug-1' => 1700000000,
    'item-slug-2' => 1690000000
);

The nested approach is a bit more self documenting and extensible, which I do like. However, there's something to be said for consistency. Seeing as it's essentially the same conceptual shape maybe we should go for that (the simple, non-nested array) in the first iteration at least. This would also have a marginal benefit to the page weight as we'll be including the serialized value on the page as a result of preloading the path (as discussed further below). WDYT?


  • [ ] get_active_items method:
    • I don't think we actually need this method. We want to provide the full value of the setting to the client, and leave the logic for checking if an item has been seen/set before, and if it's active/expired to the client. This way the client can determine whether a request needs to be made to set the timer for the item.

  • [ ] If an item with the given slug already exists, only its expires time is updated, preserving other attributes like seen. If the item is new, it is added with a default seen status of false.
    • What's the reference to "seen" here? Did you intend to add another key to the nested structure above? I'm not sure we'd need a seen though (although I do think we need an extra selector where we can determine if something has been seen, as per below, but we can derive the "seen" status from whether the item has ever been set). Anyhow, please do clarify this one.

  • [ ] GET:core/user/data/expirable-items - Callback for this route should return the items which has expired. That means the timestamp for the expiration time has been passed when we compare it with the current time.
    • As discussed above, we should ensure the full value of the setting is returned from this route, and leave additional logic to the client side.

  • [ ] POST:core/user/set-expirable-item-timers - This should accept the object consisting of slug (type: string) which represents the item and expiresInSeconds (type: int) represents the expiration time in seconds.
    • As per the AC, this route should support multiple items so we can keep the number of requests to a minimum.

  • [ ] If expiration time is not passed, set it to 0 that means it will never expire.
    • This is not a requirement here, we should always expect an expirable item to have an expiry date.

  • [ ] get_expirable_items method:
    • We won't need this method, at least not that we currently know of. So, there's no need to include it - we can add it if/when we do need it.

  • [ ] Create a store (fetchExpireItemStore) off createFetchStore.
    • As discussed above, the related endpoint should handle multiple items in a single request, so the args should be updated accordingly.
    • Also, super minor but the backticks are missing around fetchExpireItemStore, and it would be more grammatically correct to say "via createFetchStore", or "from a call to createFetchStore".
    • Please review the IB for any other backticks that have been missed in its formatting.

ankitrox commented 4 months ago

Thanks for reviewing this @techanvil .

Regarding the seen option, earlier I was writing IB based on the seen property, but later I thought that it may not be required. Setting an item in expirable items itself should be sufficient to know that it has been seen. That one point remained in IB by mistake which I've removed now.

Rest of the points I have addressed as per your suggestion.

Assigning this to you for your re-review.

Thank you.

techanvil commented 4 months ago

Thanks @ankitrox, that makes sense.

I've made a few amendments to the IB - some points were missed, and a couple of naming and grammatical tweaks were needed.

Please review the changes and if you're happy, feel free to move this to the EB.

IB :white_check_mark:

ankitrox commented 4 months ago

Looks everything good now. Thank you @techanvil .

Moving this to EB.

techanvil commented 4 months ago

Hi @ankitrox, regarding the QAB here - rather than using fetchExpirableItems() in the test, I would suggest rewriting it to use (and therefore test) setExpirableItemTimers().

Please can you also include instructions for testing hasExpirableItem()?

ankitrox commented 4 months ago

Hi @techanvil ,

Thanks for the CR. I have addressed the comments in the PR and also updated the QAB to include setExpirableItemTimers action instead of fetchExpirableItems.

Assigning this to you for review again.

Thanks

techanvil commented 4 months ago

Thanks @ankitrox! The QAB is looking good, I've left a few more comments on the PR.

kelvinballoo commented 4 months ago

QA Update ✅

Tested per the QAB and results were as expected:

Moving ticket to Approval.