Developer Features: Add Tests for Analytics Tracking

[Aurorum]

Related to #87474

Proposed Changes

This simplifies/fixes the analytics tracking for Developer Features. I couldn't actually reproduce the incorrect data, but @wongasy reported it here: https://github.com/Automattic/wp-calypso/pull/87474#issuecomment-1958461911.

~~Originally, this wasn't hardcoded because there was no need for an id - that was added in the last minute in order to avoid errors when mapping the features list. As the only condition for the id is that it's unique, I see no reason why we can't save a bit of code and use it for recording analytics too. That also guarantees no wrong analytics if the link structure changes in future.~~

As per discussion below, this now just adds tests for the existing approach rather than changing it. This ensures that analytics won't break in future.

Testing Instructions

Run yarn test-client client/me/developer/features/test/use-handle-click-link.js and confirm that they pass. You can change the feature slug in the tests locally to confirm that they fail when incorrect.

cc @nightnei, @oswian, @dlind1

[nightnei]

@Aurorum first of all, thanks for proposing PR with improvement. It's always cool to discuss any refactoring and make our codebase better.

Of course it's my subjective opinion, but I see some disadvantages with such approach:

1. The main my point - with the current approach, we have consistency - we have a separate handler that always works the same way with links - takes the href and handles it to provide the feature property. And with proposed approach we don't have consistency - sometimes we use id, sometimes we use hardcoded string and sometimes (for example the last one item) we have id but it's not used for analytics, since we don't have Learn more link.
2. Less critical and mostly subjective - with the current approach, we have a separate handler where all logic happens, so the actual components look clearer - there is only the handler name, so it's much easier to read such components.
3. Also, using href was discussed and it was agreed, so now it can mess the analytics. So at least we should keep the same feature names.

In general, sometimes less code looks harder to read, and readability is much more critical than number of lines, but of course it's subjective opinion and it depend on the specific case. In this case, I would keep what we have.

@wongasy WDYT?

[wongasy]

I couldn't actually reproduce the incorrect data, but @wongasy reported it here: https://github.com/Automattic/wp-calypso/pull/87474#issuecomment-1958461911.

I suspect the faulty data I saw were sent before this commit https://github.com/Automattic/wp-calypso/pull/87474/commits/9fba17ba1bb4f1ad282f6be44f33cd627d796be3. Looks like we are good in production.

[wongasy]

In general, sometimes less code looks harder to read, and readability is much more critical than number of lines

I agree. Although, in this case, I do find the changes proposed in this PR easier to read. Namely, we don't have to bother with useCallback, nor do we have to worry about extra logic to strip the feature name from the URL. But like you said, it could be a personal preference.

with the current approach, we have consistency - we have a separate handler that always works the same way with links - takes the href and handles it to provide the feature property.

On the other hand, I also take the point that, with the new approach, we end up with two click handlers. If our tracking logic changes, it's easy to forget to update both handlers.

Given that we've confirmed that nothing is broken. Let's keep things as-is. We'll refactor as needed when we encounter scenarios that require a more robust mechanism for tracking. For now, the priority is to ensure we're tracking the correct data, which has been confirmed!

[Aurorum]

Yes, I think it's down to personal preference - that reasoning makes sense. :) If we're definitely set on this approach though, I think it's worth adding unit tests so that this doesn't happen again, as per https://github.com/Automattic/wp-calypso/pull/87474#pullrequestreview-1883998182. That at least ensures the analytics data doesn't change in future, since it can be easy to miss for someone not familiar with this section!

What do you both think of this PR now? :)

[Aurorum]

Sorry, just noticed I committed the wrong version 🤦 This one should be good to review, and would've caught the issue.

cc @oswian if that's what you had in mind. :)

[oswian]

cc @oswian if that's what you had in mind. :)

Thanks for working on the tests @Aurorum.

I have a few suggestions:

• Avoid re-implementing the useHandleClickLink logic inside the test itself. We want to test the behaviour of the hook without making assumptions about the underlying implementation.
• Remove the call to useFeaturesList. The list of features may change over time, and while we could list all of the support URL's we know about today, I would approach writing the test in a more generic way ie. focus on how it handles the 'shape' of different URL's.
• Clarify and be specific regarding the intent of the test in the description and by providing clear inputs and outputs

Below is a test which I think can replace the two you proposed. The URL's I've included are variations that reflect the shape of the support URL's I'm aware of, but feel free to add/change.

describe( 'useHandleClickLink', () => {
	...

	describe( 'tracks event is recorded without the /support/ prefix', () => {
		test.each( [
			[ 'https://wordpress.com/support/feature', 'feature' ],
			[ 'https://wordpress.com/support/category/feature', 'category/feature' ],
			[ 'https://wordpress.com/support/feature-with-hyphens', 'feature-with-hyphens' ],
			[ 'https://wordpress.com/support/supported/feature', 'supported/feature' ],
		] )(
			"for the URL %s a tracks event is recorded using the 'feature' value '%s'",
			( href, feature ) => {
				const { result } = renderHook( () => useHandleClickLink() );
				const event = { currentTarget: { href } };

				act( () => result.current( event ) );

				expect( mockRecordTracksEventWithUserIsDevAccount ).toHaveBeenCalledWith( tracksHandle, {
					feature,
				} );
			}
		);
	} );
} );

Test run output:

 PASS  client/me/developer/features/test/use-handle-click-link.js
  useHandleClickLink
    tracks event is recorded without the /support/ prefix
      ✓ for the URL https://wordpress.com/support/feature a tracks event is recorded using the 'feature' value 'feature' (9 ms)
      ✓ for the URL https://wordpress.com/support/category/feature a tracks event is recorded using the 'feature' value 'category/feature' (1 ms)
      ✓ for the URL https://wordpress.com/support/feature-with-hyphens a tracks event is recorded using the 'feature' value 'feature-with-hyphens' (1 ms)
      ✓ for the URL https://wordpress.com/support/supported/feature a tracks event is recorded using the 'feature' value 'supported/feature' (1 ms)

[Aurorum]

Thanks @oswian! Totally agree a more generic test is better, and I've added your recommended proposal now.

Avoid re-implementing the useHandleClickLink logic inside the test itself. We want to test the behaviour of the hook without making assumptions about the underlying implementation.

I thought it might be helpful if I clarified that my intention for that test was to prevent a repeat of #87474, which is why I tested the links in useFeaturesList. The idea is that it would prevent someone adding a link with the wrong structure that would break the analytics in future.

I've removed it now, but based on that, let me know if you think it'd still make sense to include. :)

[oswian]

Thanks for addressing my feedback @Aurorum.

I thought it might be helpful if I clarified that my intention for that test was to prevent a repeat of https://github.com/Automattic/wp-calypso/pull/87474, which is why I tested the links in useFeaturesList. The idea is that it would prevent someone adding a link with the wrong structure that would break the analytics in future. I've removed it now, but based on that, let me know if you think it'd still make sense to include. :)

Thanks for highlighting this, and great catch. Our current test checks for the removal of the .../support/ prefix. We also want a test to check that trailing /'s are removed.

My suggestion would be to add another test which focuses on that case. It can be almost identical to the existing test, with the only change being the URL's under test which have trailing /'s.

It's tempting to want to reduce the duplication between the tests, however I think tests can lose clarity and expressiveness when focusing on being concise. Keen for your thoughts though!

describe( 'ensure track events are recorded without a trailing /', () => {
    const urlsWithTrailingSlashes = [
	    [ 'https://wordpress.com/support/feature/', 'feature' ],
	    [ 'https://wordpress.com/support/category/feature/', 'category/feature' ],
	    [ 'https://wordpress.com/support/feature-with-hyphens/', 'feature-with-hyphens' ],
	    [ 'https://wordpress.com/support/supported/feature/', 'supported/feature' ],
    ];
    
    test.each( urlsWithTrailingSlashes )(
	    "for the URL %s a tracks event is recorded using the 'feature' value '%s'",
	    ( href, feature ) => {
		    const { result } = renderHook( () => useHandleClickLink() );
		    const event = { currentTarget: { href } };
    
		    act( () => result.current( event ) );
    
		    expect( mockRecordTracksEventWithUserIsDevAccount ).toHaveBeenCalledWith( tracksHandle, {
			    feature,
		    } );
	    }
    );
} );

One other important change to note: the beforeAll will need to switch to a beforeEach, in order to reset our mock per test. Without this change, you'll notice that if you comment out this line in useHandleClickLink:

featureSlug = featureSlug.replace( /^\/|\/$/g, '' );

Tests for the trailing / still pass. Not good! This is because the mock's state isn't being reset between tests. Switching to beforeEach will fix that.