RHOAIENG-9232 Create useTrackUser #3024
Conversation
|
Skipping CI for Draft Pull Request. |
|
@andrewballantyne This more or less works, but is sending the identify event repeatedly -- probably due to some React re-renders. Can you give me a hint please? |
|
Thank you @christianvogt your review was super helpful. |
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
I am afk tomorrow. Will have a look on Thursday
…On 23 Jul 2024, at 19:09, Christian Vogt wrote:
@christianvogt commented on this pull request.
> + const createReviewResource: AccessReviewResourceAttributes = {
+ group: 'project.openshift.io',
+ resource: 'projectrequests',
+ verb: 'create',
+ };
+ const [allowCreate] = useAccessReview(createReviewResource);
This makes a network request but the value isn't used in the current
implementation.
> window.clusterID = clusterID;
initSegment({
segmentKey,
enabled:
!dashboardConfig.spec.dashboardConfig.disableTracking,
}).then(() => {
- computeUserId().then((userId) => {
- fireIdentifyEvent({ anonymousID: userId });
- firePageEvent();
- });
+ fireIdentifyEvent(userProps);
This comment still holds true.
Perhaps you should capture a value to know when segment is
initialized, then use a `useEffect` on that value along with
`userProps` so that you fire an identify event whenever
'userProps.anonymousID' changes?
The old implementation didn't suffer from this issue.
--
Reply to this email directly or view it on GitHub:
#3024 (review)
You are receiving this because you authored the thread.
Message ID:
***@***.***>
|
pilhuhn
force-pushed
the
RHOAIENG-9232
branch
2 times, most recently
from
0bf2cb5
to
7ee298c
Compare
pilhuhn
force-pushed
the
RHOAIENG-9232
branch
3 times, most recently
from
e2c6220
to
41e6ad7
Compare
|
I think #3024 (comment) is now solved. |
|
/lgtm, just a rebase so looks good still |
|
/lgtm |
|
/lgtm |
|
Okay, I enabled the segment stuff on my test cluster, and then I added this PR image to it. I can confirm the identify event still run on load correctly (with the same parameters) Before vs After screenshotsthe timestamp and session id's are differentbefore:
after:
|
| const { isAdmin } = useUser(); | ||
| const [anonymousId, setAnonymousId] = React.useState<string | undefined>(undefined); | ||
|
|
||
| const [loaded, setLoaded] = React.useState(false); |
There was a problem hiding this comment.
Unnecessary to track this loaded as a separate state.
Compute it as !!anonymousId && acLoaded
| [isAdmin, allowCreate, anonymousId], | ||
| ); | ||
|
|
||
| return [props, loaded]; |
There was a problem hiding this comment.
| return [props, loaded]; | |
| return [props, acLoaded && !!anonymousId]; |
| if (!anonymousId) { | ||
| computeAnonymousUserId().then((val) => { | ||
| setAnonymousId(val); | ||
| }); | ||
| } | ||
| if (acLoaded && anonymousId) { | ||
| setLoaded(true); | ||
| } | ||
| }, [username, anonymousId, acLoaded]); |
There was a problem hiding this comment.
If username changes, you aren't recomputing the anonymous id. Although I don't think username should change and you are only going to compute the anonymous id once. But should you recompute?
If you don't care to recompute based on username changes, then you can remove all hook dependencies.
| if (!anonymousId) { | |
| computeAnonymousUserId().then((val) => { | |
| setAnonymousId(val); | |
| }); | |
| } | |
| if (acLoaded && anonymousId) { | |
| setLoaded(true); | |
| } | |
| }, [username, anonymousId, acLoaded]); | |
| computeAnonymousUserId().then((val) => { | |
| setAnonymousId(val); | |
| }); | |
| // compute anonymousId only once | |
| // eslint-disable-next-line react-hooks/exhaustive-deps | |
| }, []); |
There was a problem hiding this comment.
The usename is set at login. So I don't think it will ever change during one session.
| loaded, | ||
| segmentKey, | ||
| username, | ||
| dashboardConfig, |
There was a problem hiding this comment.
Remove dashboardConfig from the list of dependencies because it's too broad.
Assign dashboardConfig.spec.dashboardConfig.disableTracking to a local variable and use it as a dependency instead.
| anonymousID?: string; | ||
| userId?: string; |
There was a problem hiding this comment.
userId isn't used anywhere. Should this be removed? I also thought that we shouldn't include identifiable information in events.
There was a problem hiding this comment.
It could indeed be removed from this PR. UserId would be the web-user-id for the sandbox case.
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jeff-phillips-18, manosnoam The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@christianvogt This was merged in the meanwhile. Should I open a subsequent PR with your suggestions? |
|
@pilhuhn Yes please open a new PR for additional changes. This shouldn't have been merged. |
Closes https://issues.redhat.com/browse/RHOAIENG-9232
Description
Put obtaining user properties into its own hook. This is meant to be extended in the future.
How Has This Been Tested?
Manual testing by developer by inspecting browser console and (for non-dev mode) network tab.
Test Impact
Request review criteria:
Self checklist (all need to be checked):
After the PR is posted & before it merges:
maincc @andrewballantyne