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

Jump to bottom

Replace confusing GeoIP warnings #16427

Closed
Findus23 opened this issue Sep 11, 2020 · 3 comments · Fixed by #16780
Closed

Replace confusing GeoIP warnings #16427

Findus23 opened this issue Sep 11, 2020 · 3 comments · Fixed by #16780
Assignees
@sgiehl
Labels
c: Onboarding For issues that make the experience of getting Matomo up and running better.
Milestone
4.1.0

Comments

@Findus23
Copy link
Member

I looked into the GeoIP system check and I am completly confused by both the code and the warnings Matomo gives:

When I set up Matomo with the default geolocation provider, I get the following warning in the system check:
Geolocation works, but you are not using one of the recommended providers. If you have to import log files or do something else that requires setting IP addresses, use the PHP GeoIP 2 implementation</a> and install <a href="https://matomo.org/docs/geo-locate/" rel="noreferrer noopener" target="_blank">maxminddb extension.
The HTML is broken and it recommends users to install maxminddb, something nearly nobody should need to do.

Looking at the code doesn't really clear anything up:

matomo/plugins/UserCountry/Diagnostic/GeolocationDiagnostic.php

Lines 45 to 65 in 1155273

$isNotRecommendedProvider = in_array($currentProviderId, array(
LocationProvider\DefaultProvider::ID,
GeoIp2\ServerModule::ID));
$isProviderInstalled = (isset($allProviders[$currentProviderId]['status']) && $allProviders[$currentProviderId]['status'] == LocationProvider::INSTALLED);
if (!$isNotRecommendedProvider && $isProviderInstalled) {
return array(DiagnosticResult::singleResult($label, DiagnosticResult::STATUS_OK));
}
if ($isProviderInstalled) {
$comment = $this->translator->translate('GeoIp2_GeoIPLocationProviderNotRecommended') . ' ';
$message = 'GeoIp2_LocationProviderDesc_ServerModule2';
$comment .= $this->translator->translate($message, array(
'<a href="https://matomo.org/docs/geo-locate/" rel="noreferrer noopener" target="_blank">', '', '', '</a>'
));
} else {
$comment = $this->translator->translate('UserCountry_DefaultLocationProviderDesc1') . ' ';
$comment .= $this->translator->translate('UserCountry_DefaultLocationProviderDesc2', array(
'<a href="https://matomo.org/docs/geo-locate/" rel="noreferrer noopener" target="_blank">', '', '', '</a>'
));
}

The warning I expect most people to see there (DefaultLocationProviderDesc1) is only displayed if !$isProviderInstalled which only happens if the status of the currently selected geolocation provider is not INSTALLED, but the default provider is obviously always installed, so I think noone ever sees the warning that provides the actual help.

I also can't think of a combination of mixed up translation strings or conditions that would make sense.

@sgiehl, do you know more about how this is supposed to work?
@Findus23 Findus23 added the c: Onboarding For issues that make the experience of getting Matomo up and running better. label Sep 11, 2020
@Findus23 Findus23 added this to the 4.0.0 RC milestone Sep 11, 2020
@tsteur
Copy link
Member

tsteur commented Sep 13, 2020

@Findus23 is this as a result of the Matomo 4 upgrade or was this maybe working before?

@Findus23
Copy link
Member Author

The only change I can see in those files is https://github.com/matomo-org/matomo/pull/15521/files#diff-37504b398acb93a27b5550d760b3c96d, but that only moved the translations and didn't change them (I think).

And indeed switching to the default geolocation in 3.14.1 shows the same message.

@tsteur tsteur modified the milestones: 4.0.0 RC, 4.1.0 Sep 15, 2020
@sgiehl
Copy link
Member

sgiehl commented Nov 24, 2020

I guess that check is already a few years old. So might make sense to complete rewrite that with more useful thing.

So imho we should have these types of results:

  • Recommended Provider installed and activated
    --> Result: OK

  • Currently selected Provider has an error
    --> Result: Error (with error message from provider)

  • Default provider is in use
    --> Result: Warning (Suggest to use another provider that does real geolocation)

  • Server module is used
    --> Result: Warning (Suggest using PHP provider if log import will be used)

@sgiehl sgiehl self-assigned this Nov 24, 2020
@sgiehl sgiehl mentioned this issue Nov 24, 2020
Merged
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@sgiehl sgiehl

Labels
c: Onboarding For issues that make the experience of getting Matomo up and running better.
Projects
None yet
Milestone
4.1.0
Development

Successfully merging a pull request may close this issue.

Improves Geolocation diagnostics matomo-org/matomo
3 participants
@tsteur @sgiehl @Findus23