-
Notifications
You must be signed in to change notification settings - Fork 294
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.
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
Infrastructure/#7458 - Create API endpoints and Redux store infrastructure for "Enable enhanced measurement" feature #7561
Infrastructure/#7458 - Create API endpoints and Redux store infrastructure for "Enable enhanced measurement" feature #7561
Conversation
…e currently selected data stream.
Fix set state logic.
Add period at the end of comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @hussain-t, nice work here. I've left a number of comments, the majority of which are very minor. Please take a look :)
assets/js/modules/analytics-4/datastore/enhanced-measurement.js
Outdated
Show resolved
Hide resolved
assets/js/modules/analytics-4/datastore/enhanced-measurement.test.js
Outdated
Show resolved
Hide resolved
assets/js/modules/analytics-4/datastore/enhanced-measurement.test.js
Outdated
Show resolved
Hide resolved
Revert the immutable logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, nice work @hussain-t!
* | ||
* @var PropertiesEnhancedMeasurementResource | ||
*/ | ||
public $properties_enhancedMeasurements; // phpcs:ignore WordPress.NamingConventions.ValidVariableName |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@techanvil I noticed this during review after it was already merged and perhaps it's inconsequential, but this is an example where the naming is inconsistent as was flagged in the original IBR (plural/singular).
Secondarily, the resource (datastream) is a sub-resource of the property but this not reflected. We should already have the class for properties->datastreams
, so perhaps we can extend the class in the library instead? This way once the library is updated to a version which includes it, we wouldn't need to change its usage. See https://gist.github.com/aaemnnosttv/ede786f6caa7679cac981669c2f90cb5#file-googleanalyticsadmin-php-L1209
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@techanvil I noticed this during review after it was already merged and perhaps it's inconsequential, but this is an example where the naming is inconsistent as was flagged in the original IBR (plural/singular).
Hi @aaemnnosttv, thanks for spotting this - this one slipped through the net in CR. AFAICT it's the only instance where we use the inconsistent plural naming. Tbh it's pretty marginal but I'm happy to create a followup issue to address if you think it's worthwhile doing so, please let me know (or we could just include it in some other future PR)...
Secondarily, the resource (datastream) is a sub-resource of the property but this not reflected. We should already have the class for
properties->datastreams
, so perhaps we can extend the class in the library instead? This way once the library is updated to a version which includes it, we wouldn't need to change its usage. See https://gist.github.com/aaemnnosttv/ede786f6caa7679cac981669c2f90cb5#file-googleanalyticsadmin-php-L1209
Hmm, I'm a bit unclear what version your gist represents... In our current version, v0.277.0
, the PropertiesDataStreams
class is used for the v2alpha
API, and I'd be hesitant to reuse it for the v1alpha
EM API methods that we're reintroducing here. I did consider extending PropertiesWebDataStreams
, but that would mean we're bringing some additional methods in that support the v1alpha
data stream API & associated classes that we don't need.
I thought it would be cleaner to simply copy the stuff we do need from the old version of PropertiesWebDataStreams
instead into a new class. With these being v1alpha
methods, I'm not sure how likely it would be for it to be reintroduced in a compatible manner in PropertiesDataStreams
anyway, and I would imagine it's more likely we'll need to rework our implementation a bit when the beta API does provide support for EM.
That's my thinking here anyway, but I'm certainly open to suggestion if you want to take a different course with this.
Summary
Addresses issue:
Relevant technical choices
getAnalyticsConfigByMeasurementIDs
selector's tests failed. The selector was directly mutating the Redux state, which conflicts with our usage ofcreateReducer
from Immer.EnhancedMeasurementSettingsModel
class, a copy of theGoogleAnalyticsAdminV1alphaEnhancedMeasurementSettings
class from the Google API client library. Please refer to this comment.PR Author Checklist
Do not alter or remove anything below. The following sections will be managed by moderators only.
Code Reviewer Checklist
Merge Reviewer Checklist