UX: Show if MaxMind key is missing on IP lookup

[MichaIng]

as discussed in https://meta.discourse.org/t/maxminddb-not-found-error/148512/7.

This also aims to mute the related error messages in logs, when Discourse tries to open the two non-existent MaxMind database files.

I just read the note about the tests. I'm completely new to Ruby and qunit, so would have to look into it. This is currently completely untested since I need to find out first how to apply best to a running Discourse instance 😉.

[CLAassistant]

All committers have signed the CLA.

[discoursebot]

This pull request has been mentioned on Discourse Meta. There might be relevant details there:

https://meta.discourse.org/t/maxminddb-not-found-error/148512/9

[MichaIng]

Conflicts resolved.

[MichaIng]

I applied the changes to our running container. After a restart, the change to lib/discourse_ip_info.rb worked well, the database file calls are skipped and no error messages show up. The change to app/assets/javascripts/admin/addon/templates/components/ip-lookup.hbs however has no effect. I think it needs to be recompiled somehow?

[nattsw]

@MichaIng the changes look fine but could you check out the test failures please?

[MichaIng]

I guess the GeoIP results are mocked. The license key needs to be defined, at least with a dummy value. I'll have a look.

EDIT: Okay, a hardcoded database is used for the test. I added a dummy license key. I hope defining a GlobalSetting value there works just like this.

[MichaIng]

Okay defining a dummy value just like that does not work:

  1) Admin::UsersController#ip_info when logged in as a moderator retrieves IP info
     Failure/Error: GlobalSetting.maxmind_license_key = "dummy"

     NoMethodError:
       undefined method `maxmind_license_key=' for GlobalSetting:Class
     Shared Example Group: "IP info retrieval possible" called from ./spec/requests/admin/users_controller_spec.rb:1667
     # ./spec/requests/admin/users_controller_spec.rb:1637:in `block (4 levels) in <main>'
     # ./spec/rails_helper.rb:358:in `block (2 levels) in <top (required)>'
     # ./vendor/bundle/ruby/3.2.0/gems/webmock-3.18.1/lib/webmock/rspec.rb:37:in `block (2 levels) in <top (required)>'

So GlobalSetting is a class, and it looks like maxmind_license_key is not a defined method if no license key was defined before compiling it?

Of course we could define DISCOURSE_MAXMIND_LICENSE_KEY in the workflows environment, but I fear that at some point it is attempted to actually authenticate with the defined key at MaxMind, which would of course fail. So I think it is better to define the dummy key just before the test. class_eval seems to work to define an additional methods. I'll try that first.

Another approach would be to use a valid MaxMind license key via GitHub Actions secret.

[MichaIng]

Okay, I did now assume that GlobalSetting.maxmind_license_key is a method (obviously not a defined variable as of above error) which prints the @@maxmind_license_key class variable. And that if the class variable @@maxmind_license_key is assigned a non-empty value, the method GlobalSetting.maxmind_license_key.blank? (which is from Discourse' own helper functions https://github.com/discourse/discourse/blob/main/lib/onebox/helpers.rb#L193) returns true.

If this does not work, then I need help from someone with more code insights and/or Ruby knowledge to have GlobalSetting.maxmind_license_key.blank? return true for this test.

EDIT: It still fails: https://github.com/MichaIng/discourse/actions/runs/4851718170/jobs/8645854302?pr=2
Hence I need help as described above. Alternative attempt still is to define DISCOURSE_MAXMIND_LICENSE_KEY in the test workflow.

EDIT2: Finally I was able to fix it: https://github.com/MichaIng/discourse/actions/runs/4855957283/jobs/8655294879
Not beautiful IMO. Since without a MaxMind license key, GlobalSetting.maxmind_license_key is not defined at all, Rspec mocks and stubs cannot be used but the GlobalSetting class needs to be extended. This needs to be done in every test where DiscourseIpInfo.open_db is called and expects a result. I'll create an alternative commit where an actual dummy license key is defined for the container build.

[SamSaffron]

Let's hold off on merging till we improve global setting mocking in the specs. Its a tiny change but I don't want to carry this legacy, its a bit confusing.

[MichaIng]

@SamSaffron

Let's hold off on merging till we improve global setting mocking in the specs. Its a tiny change but I don't want to carry this legacy, its a bit confusing.

Do you mean the currently used method with "legacy" and confusing? To me this looks quite clean:

global_setting "maxmind_license_key", "dummy"

Or what would you improve for mocking/changing global settings in tests?

[SamSaffron]

sorry for the delay here, looking good, tests are not passing though cause this needs a rebase... I hate to delay you even more on this change... will take the risk here and merge it and followup if needed.

Thanks @MichaIng