Skip to content

Undo geocoder upgrade, we must wait for leaflet-tilelayer-here#5664

Merged
tramuntanal merged 3 commits intomasterfrom
downgrade/geocoder-must_wait_leaflet_upgrade
Jan 30, 2020
Merged

Undo geocoder upgrade, we must wait for leaflet-tilelayer-here#5664
tramuntanal merged 3 commits intomasterfrom
downgrade/geocoder-must_wait_leaflet_upgrade

Conversation

@tramuntanal
Copy link
Copy Markdown
Contributor

@tramuntanal tramuntanal commented Jan 28, 2020

🎩 What? Why?

To upgrade geocoder we must wait for leaflet-tilelayer-here.
The partial geocoder upgrade done in commit 05f69836e8f9db5843c7a6b12f29bbe917216f3a must be undone because Decidim must wait until leaflet's Here plugin also supports the new Here maps API.

There's already an open issue in this plugin to support the new Here API's api_key:
https://gitlab.com/IvanSanchez/Leaflet.TileLayer.HERE/issues/3.

There's already a blocked PR in Decidim to finish the upgrade: #5644

Until leaflet also supports the new Here API, Decidim must keep on using the old app_id and app_code.

📌 Related Issues

📋 Subtasks

  • Add CHANGELOG entry
  • Add documentation regarding the feature
  • Add/modify seeds
  • Add tests
  • Another subtask

@tramuntanal tramuntanal changed the title To upgrade geocoder we must wait for leaflet-tilelayer-here Undo geocoder upgrade, we must wait for leaflet-tilelayer-here Jan 28, 2020
@microstudi microstudi self-requested a review January 29, 2020 09:15
Copy link
Copy Markdown
Contributor

@microstudi microstudi left a comment

Choose a reason for hiding this comment

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

I think we should take over the library and add it directly Decidim.
https://gitlab.com/IvanSanchez/Leaflet.TileLayer.HERE/blob/master/leaflet-tilelayer-here.js
Last commit is 3 years old, and it's quite simple. Otherwise I fear is unmaintained.

@mrcasals
Copy link
Copy Markdown
Contributor

With the current master, if you create a new app and run bundle install it will install geocoder 1.6, since no restriction is set.

I suggest changing the dependency to this:

s.add_dependency "geocoder", "~> 1.5.2"

And release a patch version. This way geocoder will keep to the 1.5.x release.

Also, after upgrading to geocoder 1.6 some configuration needs to be changed:

@microstudi
Copy link
Copy Markdown
Contributor

You're right. We will need to address soon the leaflet issue anyway, we might add the geocoder configuration upgrade in the same task.

@mrcasals
Copy link
Copy Markdown
Contributor

Please note that until you fix this, any new Decidim app will install geocoder 1.6, but since the configuration will be outdated the geocoding service will not work if we use Here.com as a provider.

Copy link
Copy Markdown
Contributor

@microstudi microstudi left a comment

Choose a reason for hiding this comment

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

We should do what @mrcasals said, bundle update needed

@tramuntanal
Copy link
Copy Markdown
Contributor Author

Thanks @microstudi and @mrcasals we already have #5644 on the way... and waiting...

@mrcasals
Copy link
Copy Markdown
Contributor

@tramuntanal but #5644 is blocked until leaflet is fixed, right? I'm suggesting merging this one (#5664) so that geocoding keeps working until #5644 is merged 😄

@tramuntanal tramuntanal requested a review from mrcasals January 30, 2020 15:48
@tramuntanal tramuntanal merged commit d88b3ea into master Jan 30, 2020
@tramuntanal tramuntanal deleted the downgrade/geocoder-must_wait_leaflet_upgrade branch January 30, 2020 18:26
@mrcasals
Copy link
Copy Markdown
Contributor

mrcasals commented Mar 2, 2020

Upgrading apps to 0.20 keeps generating problems, because it updates geocoder to the lastest version, which fails. Can you release a 0.20.1 version for decidim including the fix in this PR, please?

@mrcasals
Copy link
Copy Markdown
Contributor

mrcasals commented Mar 2, 2020

Alternatively, could you port this fix to the 0.20-stable branch? 🙏

mrcasals pushed a commit that referenced this pull request Mar 3, 2020
* To upgrade geocoder we must wait for leaflet-tilelayer-here

There's and open issue to support the new Here API's api_key:
https://gitlab.com/IvanSanchez/Leaflet.TileLayer.HERE/issues/3.

* Force geocoder version to increase only between v1.5 patches
@mrcasals mrcasals mentioned this pull request Mar 3, 2020
tramuntanal added a commit that referenced this pull request Mar 6, 2020
* Undo geocoder upgrade, we must wait for leaflet-tilelayer-here (#5664)

* To upgrade geocoder we must wait for leaflet-tilelayer-here

There's and open issue to support the new Here API's api_key:
https://gitlab.com/IvanSanchez/Leaflet.TileLayer.HERE/issues/3.

* Force geocoder version to increase only between v1.5 patches

* Add changelog entry

Co-authored-by: Oliver Valls <oliver.vh@coditramuntana.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants