Open zutigrm opened 3 weeks ago
@tofumatt any chance you can review this one please?
@zutigrm Which instances of validateHaveSettingsChanged
should be changed here?
I see at least two separate instances:
There's the default created for this argument: https://github.com/google/site-kit-wp/blob/81fdd2fb31d7d9bd529bfcde292754db1d01c00a/assets/js/googlesitekit/data/create-settings-store.js#L85, but I don't think that is relevant here.
properly validate selector for function throw, and use returned selectors
What does this mean exactly? It seems like it means that it should try/catch
a callback and return its value if it doesn't catch, but I'm not clear on what this means. Are there other functions you can supply as examples?
@tofumatt I see your point, I will make it clearer, it is basically about how the function is used in the store. Will update the AC
@tofumatt AC updated
The ACs needed more clarity around what the outcome should be here, so mentioning the explicit selector names and where they'd come from I think clears things up.
Moving to IB 🙂
switching the conversion tracking toggle on edit settings screen of Ads and Analytics module should change be detected in the
save
button
Can you rephrase this? I don't understand what this means specifically.
The IB here doesn't go into any details about what should change really, aside from what's already outlined in the ACs. Can you go into at least a bit of conceptual detail on how this would be implemented/how it should function? Right now the IB just reads like "implement the ACs", which isn't specific enough to review.
For example, what should be tested in the tests for these selectors? What is the behaviour we want to test? That would be good to outline in the IB.
@tofumatt Thanks. Since it straightforward and AC covered good part of the details with pointing to the example, I thought it won't need much details in IB as well. I updated IB now to have more info and expanded on test section, let me know what you think
IB ✅
@zutigrm I completed QA according to QAB but AC of this ticket is more technical. Should we tag QA:Eng
to verify points mentioned under AC ?
Feature Description
validateHaveSettingsChanged
should be used withcreateValidationSelector
so it can be properly validated agains functions throw, and also test coverage for this utility should be added. More details in the comment hereDo not alter or remove anything below. The following sections will be managed by moderators only.
Acceptance criteria
haveSettingsChanged
selector inassets/js/googlesitekit/data/create-settings-store.js
should utilise thecreateValidationSelector
utility function to properly validate thevalidateHaveSettingsChanged
function.assets/js/googlesitekit/data/create-settings-store.js
should be updated to have bothhaveSettingsChanged
and__dangerousHaveSettingsChanged
selectors, as done in other files that usecreateValidationSelector
.haveSettingsChanged
selector should be thesafeSelector
returned fromcreateValidationSelector( validateHaveSettingsChanged )
, while the__dangerousHaveSettingsChanged
should be thedangerousSelector
returned fromcreateValidationSelector( validateHaveSettingsChanged )
.validateHaveSettingsChanged
should be added.validateHaveSettingsChanged
is used by both selectors (see https://github.com/google/site-kit-wp/blob/3b3ebf91cdb01a0670e30a5cfd2afba0778a9a11/assets/js/googlesitekit/modules/create-submit-changes-store.test.js#L152-L165 for an example of how to test this).Implementation Brief
assets/js/googlesitekit/data/create-settings-store.js
haveSettingsChanged
selector https://github.com/google/site-kit-wp/blob/3b3ebf91cdb01a0670e30a5cfd2afba0778a9a11/assets/js/googlesitekit/data/create-settings-store.js#L292-L296 which uses passed propvalidateHaveSettingsChanged
directly without validating it, to follow the existing semantic convention we have in other places, by utilising thecreateValidationSelector
which will catch any errors thrown by the selector.validateHaveSettingsChanged
prop to thecreateValidationSelector
util function, and extract returned selectorscreateValidationSelector
will returnsafeSelector
- which catches exceptions, anddangerousSelector
which does not catch exceptions (eg. will throw with errors). Use aliashaveSettingsChanged
forsafeSelector
and__dangerousHaveSettingsChanged
fordangerousSelector
and add them to the main selectors object . You can see an example usage here https://github.com/google/site-kit-wp/blob/3e1657a3a3a10572812617bc45e91de4752bdac1/assets/js/googlesitekit/modules/create-submit-changes-store.js#L146-L153Test Coverage
haveSettingsChanged
. You can useassets/js/googlesitekit/modules/create-submit-changes-store.test.js
canSubmitChanges
as an example, to verify thathaveSettingsChanged
and__dangerousHaveSettingsChanged
are functions and that they are usingvalidateCanSubmitChanges
https://github.com/google/site-kit-wp/blob/3b3ebf91cdb01a0670e30a5cfd2afba0778a9a11/assets/js/googlesitekit/modules/create-submit-changes-store.test.js#L143-L165QA Brief
Changelog entry