fix: 3679 - reporting with a 000000000000 matomo user id

[sgtSeme4ki]

What

• If a user opts out from analytics then track with a 000000000000 user-id

Fixes bug(s)

• Fixes: Reporting with a 000000000000 matomo user id #3679

Part of

• Matomo (Tracker) #2855

[monsieurtanuki]

Hi @sgtSeme4ki!
Please have a look at my quick review.

[monsieurtanuki]

Suggested change

	static bool allow = false;
	static bool _allow = false;

[monsieurtanuki]

Please add a comment if those are dummy values.

[monsieurtanuki]

Suggested change

	expect(matomoUserId, '0000000000000000');
	expect(matomoUserId, '0' * 16);

[sgtSeme4ki]

@monsieurtanuki Thank you for the review, I tried to resolve anything as far as possible

[monsieurtanuki]

Hi @sgtSeme4ki, and thank you for your additional commits.
Please have a look at my comments; basically your code is good but I still have pending questions.

[monsieurtanuki]

Suggested change

	// if user opts out then track anonymously with userId containg zeros
	if (!_allow) {
	return '0' * 16;
	} else if (kDebugMode) {
	return 'smoothie_debug--';
	} else {
	return OpenFoodAPIConfiguration.uuid;
	}
	if (kDebugMode) {
	return 'smoothie_debug--';
	}
	// if user opts out then track anonymously with userId containg zeros
	if (!_allow) {
	return '0' * 16;
	}
	return OpenFoodAPIConfiguration.uuid;

I believe we want to track "real users", not developers debugging.
That's why I would put the kDebugMode check first.
In addition to that I don't like elses so as we return we can get rid of them, but it's just a matter of personal taste.
The most important here is to check kDebugMode first.

[sgtSeme4ki]

If kDebugMode is checked first, the test always fails, because tests are always run in debug mode (always returns "smoothie_debug--". Do you have an idea how to overcome this? My first thought was setting an env-variable like TEST_MODE in the test and checking for it in the "get uuid"-method, but this seems dirty to me. But honestly I can't come up with something better.

[monsieurtanuki]

I understand your point, but you seem to be more interested in the test than in the code.
The code should match our needs, and that's why I would put the kDebugMode check first.

If there's no way to test it properly, let's not test it. Especially in that case, it's just a if we're checking. Of course a test would be better; it's not essential.

If really you insist, perhaps you could use @visibleForTesting. But I wouldn't put too much effort into it, and I would discard the test altogether.

[sgtSeme4ki]

Thank you for your input. Am always interested in tests, because I come from a strict TDD-Household and the guys at the company I work in regard the tests as holy ;). But the most pragmatic way is to drop the test and it probably isn't worth it for like just 5 lines of code. I removed the test and the mockPackageInfo().

[monsieurtanuki]

Let's be pragmatic 😉

[monsieurtanuki]

Thank you @sgtSeme4ki and congratulations for your first contribution here!