Get registration location map working with OSM

[cillian]

What? Why?

This add two things to get a bit closer to #5542

(1) It prevents a broken map from appearing during registration if an instance has a GOOGLE_MAPS_API_KEY environment variable present i.e. they already have Google Maps enabled AND that instance then enables Open Street Map (OSM). This issue was spotted here. The fix for this is in the using_google_maps? method.

(2) It gets the registration location map working with OSM, see:

osm-registration-map.mp4

What should we test?

• Step 1: Enable OSM
  • Sign in as a super admin and go to the Configuration > Content section.
  • Fill in the OSM settings like in screenshot below. With OSM you can choose between a number of different tile providers. In this example MapBox is used, you can create an API key with them for free)
    
• Step 2: Register as a new enterprise and check you can set your location on the map like you can when Google Maps is enabled.

Release notes

Changelog Category (reviewers may add a label for the release notes):

• User facing changes

[rioug]

It's a nice addition, thanks.
I would like to see some test here, it's probably tricky to test the stimulus controller with Jest given the interaction with the map.But, I think we should at a minimum test that the OSM map is displayed when configured properly, which should be doable via system spec. Could you give it a go ?

[cillian]

No problem, thanks for the review. We could add a system test like below to https://github.com/openfoodfoundation/openfoodnetwork/blob/master/spec/system/consumer/registration_spec.rb ....

it "displays the location map using Open Street Maps" do
  visit registration_path
  switch_to_login_tab
 
  fill_in "Email", with: user.email
  fill_in "Password", with: user.password
  click_button "Login"
  click_button "Let's get started!"

  # The .leaflet-container class is only present if map is initialised
  expect(page).to have_selector("#open-street-map.leaflet-container")
end

But then I added view and helper specs instead because I was wondering would a system spec abuse OSM tile servers by sending requests everytime the tests are ran. Not sure if that's okay?

[rioug]

I was wondering would a system spec abuse OSM tile servers by sending requests everytime the tests are ran. Not sure if that's okay?

Indeed that's an issue, and we don't want to rely on a third party when running system specs. There is always the option of stubbing the request to OSM/google map, but I like your idea of using a view test. System test are expensive to run and it's not strictly necessary for this.
Thanks for your help 🙏 !

[dacook]

Great, thanks for implementing Leaflet/OSM!
I wondered if this new implementation could also be used for Google Maps (https://stackoverflow.com/questions/281264/remove-empty-elements-from-an-array-in-javascript).. but I think it's better to keep the official Google API.

[dacook]

FYI this could be a little neater with Optional chaining
(but it's totally fine as-is)

[dacook]

Clever fix! I'm not sure if we'll need it for other purposes, but I think it could easily become a generic method (with the element ID as a parameter).

[filipefurtad0]

Hey @cillian ,

Thank you for the detailed instructions. Proceeded as you've suggested, and was able to verify that the Open Street map is displayed when creating an enterprise:

Also, I've verified that the a change is persistent when one adjusts the suggested location (after typing in an address and clicking Locate on Map). Below, a change from the initially displayed position 1) from 2) was made (when creating an enterprise), which is then shown, when visiting the /map tab:

(apologies, no video... one was recorded with Loom, but somehow it is still uploading, and I think it does not make sense to wait longer...)

I've made a quick test to verify that no regression is introduced, i.e., that having the open street maps disabled, allows using the previous google-maps as before - all good 👍

Also, verified that enabling OpenStreetMaps with an invalid config does not break the page anymore:

This looks great, merging this PR. Thank you 💪