chore: Matomo refactor

[M123-dev]

What

For: #2855

We now have a general AnalyticsHelper.trackEvent method and not a own method per tracked action

[codecov-commenter]

Codecov Report

Merging #3273 (7599bd4) into develop (ea9904f) will increase coverage by 0.01%.
The diff coverage is 4.54%.

@@             Coverage Diff             @@
##           develop    #3273      +/-   ##
===========================================
+ Coverage    10.53%   10.54%   +0.01%     
===========================================
  Files          249      249              
  Lines        12248    12240       -8     
===========================================
+ Hits          1290     1291       +1     
+ Misses       10958    10949       -9     

Impacted Files	Coverage Δ
...oth_app/lib/data_models/continuous_scan_model.dart	0.83% <0.00%> (ø)
...ages/smooth_app/lib/helpers/launch_url_helper.dart	0.00% <0.00%> (ø)
...mooth_app/lib/pages/personalized_ranking_page.dart	0.00% <ø> (ø)
...ib/pages/preferences/user_preferences_account.dart	20.68% <0.00%> (-0.29%)	⬇️
...smooth_app/lib/pages/product/new_product_page.dart	0.00% <0.00%> (ø)
...ooth_app/lib/pages/user_management/login_page.dart	66.66% <0.00%> (-0.55%)	⬇️
...th_app/lib/pages/user_management/sign_up_page.dart	52.72% <0.00%> (-0.25%)	⬇️
...es/smooth_app/lib/query/barcode_product_query.dart	0.00% <0.00%> (ø)
...kages/smooth_app/lib/helpers/analytics_helper.dart	2.63% <12.50%> (+2.63%)	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[monsieurtanuki]

Hi @M123-dev!

Honestly I would have written things differently, in 2 major points.

I agree with the choice of reducing the number of methods. And I would even use only one method (instead of 2 for you):

  static void trackEvent(
     String name,
     String category,
     String action, {
     String? barcode,
   })

The other thing that troubles me is enum AnalyticsMessage<T extends String>, which is "just in case power 2". Just in case we use classes that extend String (spoiler alert: we won't). Just in case we use the same AnalyticsMessage twice (which does not happen very often, does it?). In addition to that sometimes you use an AnalyticsMessage for the name parameter, sometimes for the category (with a lousy .toString()). Confusion!

I don't know actually how those values are used in Matomo itself - I guess you prefer fixed values. If you really insist in using enums, you should create AnalyticsName, AnalyticsCategory and AnalyticsAction.
But given the low number of different values, I guess we would be better off with Strings like before.

Unless I'm missing an important point here. Am I?

I would also say that the name implies a category. Something like that:

enum AnalyticsCategory{
   userManagement(tag: 'user management'),
   scanning(tag: 'scanning'),
   share(tag: 'share'),
   couldNotFindProduct(tag: 'could not find product');
   
   const AnalyticsCategory({required this.tag});
   final String tag;
}

enum AnalyticsMessage{
   scanAction(tag: 'scanned product', category: AnalyticsCategory.scanning),
   shareProduct(tag: 'shared product', category: AnalyticsCategory.share),
   loginAction(tag: 'logged in', category: AnalyticsCategory.userManagement),
   registerAction(tag: 'register', category: AnalyticsCategory.userManagement),
   logoutAction(tag: 'logged out', category: AnalyticsCategory.userManagement),
   couldNotScanProduct(tag: 'could not scan product', category: AnalyticsCategory.couldNotFindProduct);
   couldNotFindProduct(tag: 'could not find product', category: AnalyticsCategory.couldNotFindProduct);

   const AnalyticsMessage({required this.tag, required this.category});
   final String tag;
   final AnalyticsCategory category;
 }

And you could build from there.

Sorry for being so verbose...

[M123-dev]

Sorry for being so verbose...

Thanks for beeing so verbose 😄

The other thing that troubles me is enum AnalyticsMessage, which is "just in case power 2". Just in case we use classes that extend String (spoiler alert: we won't)

Yeah I head some problem with the enum, I though we have to provide a type to the enum it self, but when I just used as type the constructor wouldn't work anymore.

Thanks for this detailed review, is indeed way better like this 👍🏼

[monsieurtanuki]

Hi @M123-dev: looks much better that way!
Feel free to ignore my comments and merge.

[M123-dev]

Thanks for your review @monsieurtanuki

[monsieurtanuki]

@M123-dev Perfect!