Skip to content
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

chore: Upgrade Matomo to 3.1 #4000

Merged
merged 4 commits into from
Jul 16, 2023
Merged

Conversation

g123k
Copy link
Collaborator

@g123k g123k commented May 23, 2023

The migration from Matomo 2.x to 3.x requires some minor changes in our code

@g123k g123k requested a review from a team as a code owner May 23, 2023 08:37
@github-actions github-actions bot added 📈 Analytics We use Sentry and Matomo, with an opt-in system dependencies labels May 23, 2023
@g123k g123k linked an issue May 23, 2023 that may be closed by this pull request
Copy link
Member

@teolemon teolemon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@g123k
Copy link
Collaborator Author

g123k commented May 23, 2023

I think there is an initialization issue. I make this a draft the time I investigate

@g123k g123k marked this pull request as draft May 23, 2023 08:54
@g123k
Copy link
Collaborator Author

g123k commented May 23, 2023

Ok, we can't upgrade as of now, because our tests fail.
Basically the issue is with device_info, but let me explain.

With Matomo 3.x, we have to initialize it, otherwise background calls to track screens will fail.
But to do so, it checks the user agent by using device_info.

However on macOS, it hangs on:

final macInfo = await effectiveDeviceInfo.macOsInfo;

So we are blocked by device_info to do the upgrade.
One alternative is to create our own TraceableClientMixin where we disable Matomo

@g123k g123k changed the title chore: Upgrade Matomo to 3.1 chore: Upgrade Matomo to 3.1 [BLOCKED] May 23, 2023
@g123k
Copy link
Collaborator Author

g123k commented May 23, 2023

I've opened an issue to track the progress: fluttercommunity/plus_plugins#1845

@g123k
Copy link
Collaborator Author

g123k commented Jun 10, 2023

No news from flutter community. I will wait a bit and create the PR myself, but all flutter community repos seem very dormant

@monsieurtanuki
Copy link
Contributor

However on macOS, it hangs on

@g123k I had a look at our code (user_preferences_connect.dart, user_preferences_dev_debug_info.dart) , and the only data from device_info we use is for android or iOS. Not macOS.
Perhaps we could skip the call to device_info if we're on macOS (defaultTargetPlatform == TargetPlatform.macOS). Or skip it when we're not on android or iOS.
And then, when the bug is fixed in device_info, remove our additional macOS test.

@g123k
Copy link
Collaborator Author

g123k commented Jun 27, 2023

However on macOS, it hangs on

@g123k I had a look at our code (user_preferences_connect.dart, user_preferences_dev_debug_info.dart) , and the only data from device_info we use is for android or iOS. Not macOS. Perhaps we could skip the call to device_info if we're on macOS (defaultTargetPlatform == TargetPlatform.macOS). Or skip it when we're not on android or iOS. And then, when the bug is fixed in device_info, remove our additional macOS test.

Actually the problem is not in our code, but in Matomo.
Clearly we don't release on macOS. But we use a macOS machine to run tests. Hence the issue.

@monsieurtanuki
Copy link
Contributor

Actually the problem is not in our code, but in Matomo.

@g123k Oh ok, I didn't understand that. That said:

  • we have the same problem in our own code (as I mentioned earlier)
  • perhaps it would make sense to disable matomo altogether for macOS (temporarily, until the bug is fixed)
  • perhaps it would make sense to disable matomo altogether for tests (permanently, as it may not make sense to track our tests again and again)

@g123k
Copy link
Collaborator Author

g123k commented Jul 16, 2023

I think I've finally found a way to bypass the issue.
Basically, we have to initialize Matomo, because some screen depends on it (with the dedicated mixin).

By faking the two method channels required by Matomo, we can initialize it and then run our tests

@github-actions
Copy link
Contributor

@codecov-commenter
Copy link

Codecov Report

Merging #4000 (ab4579d) into develop (9755e76) will increase coverage by 0.02%.
The diff coverage is 0.00%.

@@             Coverage Diff             @@
##           develop    #4000      +/-   ##
===========================================
+ Coverage    10.79%   10.81%   +0.02%     
===========================================
  Files          287      287              
  Lines        14216    14216              
===========================================
+ Hits          1534     1538       +4     
+ Misses       12682    12678       -4     
Impacted Files Coverage Δ
...kages/smooth_app/lib/helpers/analytics_helper.dart 6.55% <0.00%> (+6.55%) ⬆️

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

@g123k g123k marked this pull request as ready for review July 16, 2023 15:58
@g123k g123k changed the title chore: Upgrade Matomo to 3.1 [BLOCKED] chore: Upgrade Matomo to 3.1 Jul 16, 2023
@g123k g123k merged commit 7487b72 into openfoodfacts:develop Jul 16, 2023
9 checks passed
@g123k g123k deleted the matomo_upgrade branch July 16, 2023 16:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📈 Analytics We use Sentry and Matomo, with an opt-in system dependencies goldens 🧪 Tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update the Matomo calls so that we can upgrade the dependancy
4 participants