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

✨ Add new geocoding provider: Bing #646

Merged
merged 44 commits into from
Nov 24, 2023
Merged

Conversation

wlorenzetti
Copy link
Member

@wlorenzetti wlorenzetti commented Nov 8, 2023

Closes: #645

Add the capability to the end user of G3W-SUITE to set the provider for 'geocoding' map control (old 'nominatim').

I the section of 'Options and actions' of qdjango project form page, it was added a multi select box for choose the geoconding providers to use in the webgis client.

Screenshot_20231108_152611

The providers can be set by the Django settings variable GEOCONDING_PROVIDERS who the default value is:

GEOCODING_PROVIDERS = {
  "nominatim": {
    "label": "Nominatim (OSM)",
    "url": "https://nominatim.openstreetmap.org/search",
  },
  "bing_streets": {
    "label": "Bing Streets",
    "url": "https://dev.virtualearth.net/REST/v1/Locations/",
  },
  "bing_places": {
    "label": "Bing Places",
    "url": "https://dev.virtualearth.net/REST/v1/LocalSearch/",
  },
}

NB: bing providers will be available in the Geoconding providers select box if a bing key it was set inside the Django settings VENDOR_KEYS

@wlorenzetti wlorenzetti added the feature New feature or request label Nov 8, 2023
@wlorenzetti wlorenzetti added this to the v3.7 milestone Nov 8, 2023
@wlorenzetti wlorenzetti self-assigned this Nov 8, 2023
@wlorenzetti
Copy link
Member Author

@volterra79 @Raruto a this point i suggest to rename nominatim mapcontrol in geocoding. It'd be possible?

@wlorenzetti
Copy link
Member Author

@volterra79 @Raruto now in initconfig variable (or API REST) the mapcontrols property it was changed:

"mapcontrols": {
            "zoombox": {},
            "zoom": {},
            "zoomhistory": {},
            "zoomtoextent": {},
            "query": {},
            "querybbox": {},
            "querybypolygon": {},
            "querybydrawpolygon": {},
            "overview": {},
            "scaleline": {},
            "geolocation": {
                "nominatim": {
                    "label": "Nominatim (OSM)",
                    "url": "https://nominatim.openstreetmap.org/search"
                },
                "bing": {
                    "label": "Bing",
                    "url": "https://dev.virtualearth.net/REST/v1/LocalSearch/"
                }
            },
            "streetview": {},
            "nominatim": {
                "nominatim": {
                    "label": "Nominatim (OSM)",
                    "url": "https://nominatim.openstreetmap.org/search"
                },
                "bing": {
                    "label": "Bing",
                    "url": "https://dev.virtualearth.net/REST/v1/LocalSearch/"
                }
            },
            "addlayers": {},
            "length": {},
            "area": {},
            "mouseposition": {},
            "scale": {},
            "screenshot": {},
            "geoscreenshot": {}
        }

@Raruto
Copy link
Contributor

Raruto commented Nov 8, 2023

@wlorenzetti perhaps you meant "geocoding" ?

"geolocation": { // <-- "geolocation" does not require custom providers
  "nominatim": {
      "label": "Nominatim (OSM)",
      "url": "https://nominatim.openstreetmap.org/search"
  },
  "bing": {
      "label": "Bing",
      "url": "https://dev.virtualearth.net/REST/v1/LocalSearch/"
  }
},

I haven't tested it yet, but I think it's wrong within the PR code too.

@wlorenzetti
Copy link
Member Author

@wlorenzetti perhaps you meant "geocoding" ?

"geolocation": { // <-- "geolocation" does not require custom providers
  "nominatim": {
      "label": "Nominatim (OSM)",
      "url": "https://nominatim.openstreetmap.org/search"
  },
  "bing": {
      "label": "Bing",
      "url": "https://dev.virtualearth.net/REST/v1/LocalSearch/"
  }
},

I haven't tested it yet, but I think it's wrong within the PR code too.

@Raruto now it 'd work fine.

@wlorenzetti perhaps you meant "geocoding" ?

Yes :)

I haven't tested it yet, but I think it's wrong within the PR code too.

@Raruto try now it'd work fine now.

Comment on lines +177 to +178
continue
Copy link
Contributor

Choose a reason for hiding this comment

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

@wlorenzetti hiding it can become difficult to use (and discover), I would recommend showing an informational message instead, eg:

image

<div class="help-block" style="color: red">
  ⚠️ No <a href="https://g3w-suite.readthedocs.io/en/latest/settings.html#vendors-settings" target="_blank"><code>VENDOR_KEYS</code></a> found for "Bing" provider
</div>

Correct me if I'm wrong, only Nominatim provider will be activated by default for each project (so it wouldn't send invalid provider configurations to the client anyway):

image

Copy link
Member Author

Choose a reason for hiding this comment

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

@Raruto for me is correct not show Bing if a a vendor key for it is not set inside the settings. It could be useful add a description for the find int he form writing a thing like this: "For use Bing geoding is necessary have a Bing API key "

Correct me if I'm wrong, only Nominatim provider will be activated by default for each project (so it wouldn't send invalid provider configurations to the client anyway):

No why? at most you will have a map control without provider in the configuration API

Copy link
Contributor

@Raruto Raruto Nov 10, 2023

Choose a reason for hiding this comment

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

hiding it can become difficult to use (and discover), I would recommend showing an informational message instead

for me is correct not show Bing if a a vendor key for it is not set inside the settings

I said that because it complicates development a bit.

API keys (and other server configurations) can also be added on the fly via JS.

If we also have to check the docker configuration it becomes an extra step (a "duplicated" step) which would require a dedicated documentation..

Even on the server side, a sysadmin might even be happy to simply do that:

GEOCONDING_PROVIDERS = {
  "bing": {
    "label": "Bing",
    "url": "https://dev.virtualearth.net/REST/v1/LocalSearch/?key=my.super.secret.key"
  }
}

which is equivalent to the wordy version:

VENDOR_KEYS = {
 'bing': 'my.super.secret.key'
}

GEOCONDING_PROVIDERS = {
  "bing": {
    "label": "Bing",
    "url": "https://dev.virtualearth.net/REST/v1/LocalSearch/"
  }
}

But without the risk to lose them in configuration files.

In essence, let's leave people a little more freedom on how to handle it.

Copy link
Member Author

Choose a reason for hiding this comment

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

@Raruto ok I can understand what you are saying me, but the VENDORS_KEYS are used also for other features i.e. base layers .. so I think if better leave one place where to set the VENDOR key.

wlorenzetti and others added 8 commits November 15, 2023 09:38
* Add PASSWORD_CHANGE_FIRST_LOGIN setting.

* Add change_password_first_login property to Userdata model.

* Create custom G3WSetPasswordForm and custom G3WLoginView to use for change password at first login workflow.

* Start testing.

* Testing.

* Split change password at first login from reset password workflow.

* Create a custom view for confirm reset password to split User rest password workflow from Change password at first login worflow.

* Override settings fix.

---------

Co-authored-by: wlorenzetti <[email protected]>
* Add 'postgresraster' qgis layer type support.

* ⬆️ Client:
    g3w-client :g3w-suite/g3w-client#519

---------

Co-authored-by: wlorenzetti <[email protected]>
Co-authored-by: volterra79 <[email protected]>
@Raruto Raruto changed the title ✨ Set geocoding provider ✨ Add new geocoding provider: Bing Nov 15, 2023
@Raruto Raruto marked this pull request as ready for review November 15, 2023 13:32
    g3w-client: Add geocoding providers folder
@wlorenzetti wlorenzetti merged commit cab94f6 into dev Nov 24, 2023
4 checks passed
@wlorenzetti wlorenzetti deleted the Set_geocoding_provider branch November 24, 2023 07:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

✨ Set geocoding provider
3 participants