[16.0][ADD] pos_partner_location_map #1002

geomer198 · 2023-05-20T20:13:12Z

This module allows to select partner address directly on map. For this Google Maps are used.

legalsylvain

I think the name of the module should contain "google".

em230418 · 2023-05-20T21:56:07Z

pos_partner_location_map/static/src/js/googleMapInit.esm.js

+    }).then((response) => {
+        loadJS(
+            "https://maps.googleapis.com/maps/api/js?key=" +
+                response +
+                "&libraries=places"
+        );
+    });


You need to add check here if resonse is not falsy.

em230418 · 2023-05-20T21:59:05Z

pos_partner_location_map/readme/DESCRIPTION.rst

@@ -0,0 +1,2 @@
+This module allows to select partner address directly on map.
+For this Google Maps are used.


Since this module heavily depends on google, it should be renamed to something like pos_partner_location_map_google. Because there are other services like google maps. For example yandex maps. https://yandex.ru/dev/maps/

Since this module heavily depends on google, it should be renamed to something like pos_partner_location_map_google. Because there are other services like google maps. For example yandex maps. https://yandex.ru/dev/maps/
I think the name of the module should contain "google".

Hi @em230418 @legalsylvain thank you for the feedback! This module implements core behaviour for any map provide. Google Is just the first one on the list. This is why we have opted for such a name keeping OpenStreetMap as a "Roadmap/Todo" item So anyone could modify this module later without a need of creating a separate one for each map provider.

This module implements core behaviour for any map provide. Google Is just the first one on the list.

Do you plan to maintain behaviour of every map provider that is included to this module by others (1) or just google and OSM only (2)? If (2), you need to create seperate modules.

This module implements core behaviour for any map provide. Google Is just the first one on the list.

Do you plan to maintain behaviour of every map provider that is included to this module by others (1) or just google and OSM only (2)? If (2), you need to create seperate modules.

Yes, this module is designed to be extendable with other map providers as well

Sorry, my question was incorrect.

So anyone could modify this module later without a need of creating a separate one for each map provider.

Do you plan to maintain behaviour of every map provider that WILL BE included to this module (pos_partner_location_map) by others (1) or just google and OSM only (2)? If (2), you need to create separate modules.

Yes, this module is designed to be extendable with other map providers as well

I don't have any doubt on this.

Sorry, my question was incorrect.

So anyone could modify this module later without a need of creating a separate one for each map provider.

Do you plan to maintain behaviour of every map provider that WILL BE included to this module (pos_partner_location_map) by others (1) or just google and OSM only (2)? If (2), you need to create separate modules.

Yes, this module is designed to be extendable with other map providers as well

I don't have any doubt on this.

Yes if someone will open a PR with new map providers we will merge it when reviewed and approved.

ivs-cetmix · 2023-05-21T11:29:17Z

pos_partner_location_map/__manifest__.py

+    "name": "Point of Sale - Customer Screen",
+    "version": "16.0.1.0.0",
+    "category": "Point Of Sale",
+    "summary": "Point of Sale - Customers can easily interact with point of sale",


Show and set partner location directly on map in POS

ivs-cetmix · 2023-05-21T11:29:17Z

pos_partner_location_map/__manifest__.py

@@ -0,0 +1,22 @@
+{
+    "name": "Point of Sale - Customer Screen",


POS Partner Location Map

ivs-cetmix · 2023-05-21T11:32:10Z

pos_partner_location_map/models/res_partner.py

+class ResPartner(models.Model):
+    _inherit = "res.partner"
+
+    map_partner_address = fields.Char()


This field might be misleading for a user. Because there are core Odoo fields used for the same goal.
We should parse this string into Odoo address fields instead.

em230418

Also noting, that there is OCA module for google map integration. https://github.com/OCA/geospatial/tree/14.0/web_view_google_map

ivs-cetmix · 2023-05-23T10:09:53Z

Also noting, that there is OCA module for google map integration. https://github.com/OCA/geospatial/tree/14.0/web_view_google_map

Yup we are aware of. However this one is based on the Odoo core module base_geolocalize for better compatibility

geomer198 · 2023-05-30T15:44:59Z

@isserver1, @legalsylvain, @em230418 Could you make code review please?

legalsylvain · 2023-05-30T15:57:34Z

Hi @em230418 @legalsylvain thank you for the feedback! This module implements core behaviour for any map provide. Google Is just the first one on the list. This is why we have opted for such a name keeping OpenStreetMap as a "Roadmap/Todo" item So anyone could modify this module later without a need of creating a separate one for each map provider.

Not totally agree with that point. I think that if openstreetmap is in the roadmap, many modules should exists :

pos_partner_location_abstract (for common things)
pos_partner_location_google_map (for specific things).
pos_partner_location_openstreetmap (for specific things).

The module you provide is quite complex. (that is normal). So if tomorrow openstreetmap is implemented in the same module, we'll have a lot of useless code installed, that is not a good idea. (javascript asset related to google, or file like this address_google_struct.py). For maintenability reason also. If we implement osm in the same module as google, and a contributor want to migrate the module in version 17, the current design will ask to the contributor to migrate both features, even if one of the feature is not desired by the contributors.

At this step, I see two possibilities :
A) creating 2 modules, but maybe it's quite hard to split and we maybe don't know what is "common" at this step.
B) rename the current module into pos_partner_location_google_map and if an other contributor want to create a openstreetmap module, it can contribute pos_partner_location_openstreetmap_map and create a new module abstract and make the two modules (google and osm) depends on the new abstract module.

What do you think ?

geomer198 · 2023-05-30T20:19:55Z

Hi @em230418 @legalsylvain thank you for the feedback! This module implements core behaviour for any map provide. Google Is just the first one on the list. This is why we have opted for such a name keeping OpenStreetMap as a "Roadmap/Todo" item So anyone could modify this module later without a need of creating a separate one for each map provider.

Not totally agree with that point. I think that if openstreetmap is in the roadmap, many modules should exists :
* pos_partner_location_abstract (for common things)

* pos_partner_location_google_map (for specific things).

* pos_partner_location_openstreetmap (for specific things).
The module you provide is quite complex. (that is normal). So if tomorrow openstreetmap is implemented in the same module, we'll have a lot of useless code installed, that is not a good idea. (javascript asset related to google, or file like this address_google_struct.py). For maintenability reason also. If we implement osm in the same module as google, and a contributor want to migrate the module in version 17, the current design will ask to the contributor to migrate both features, even if one of the feature is not desired by the contributors.

At this step, I see two possibilities : A) creating 2 modules, but maybe it's quite hard to split and we maybe don't know what is "common" at this step. B) rename the current module into pos_partner_location_google_map and if an other contributor want to create a openstreetmap module, it can contribute pos_partner_location_openstreetmap_map and create a new module abstract and make the two modules (google and osm) depends on the new abstract module.

What do you think ?

I think that we can split the module into two separate ones pos_partner_location_base and pos_partner_location_google_map.
Can we keep both In the same pr or should make separate for each of them?

legalsylvain · 2023-05-30T22:03:59Z

Can we keep both In the same pr or should make separate for each of them?

In general, in the OCA, it's recommended to separate PRs. however, in this case, module A doesn't make sense without module B, and vice versa. so i don't see any advantage in separating, you can do the refactor in the same branch. That will be easier.

Thanks a lot!

GabbasovDinar

Not testes. code LGTM, please squash commit by modules

ivs-cetmix

Functional ok

ivs-cetmix · 2023-08-19T11:21:15Z

hey @legalsylvain @em230418 could you please check again? Would be great to have it merged

ivs-cetmix · 2023-11-28T09:01:48Z

@legalsylvain @em230418 I'm really sorry for bothering just didn't want to get it stalled

legalsylvain · 2023-11-28T11:20:22Z

Sorry for the delay. I can take a look on runboat, but there is no link. Could you rebase, to relaunch the CI ?
thanks !

legalsylvain

General review : No functional review finally, I have no Google API key. So feature doesn't work on runboat.

Nitpicking + non blocking :
The lat / long are not well displayed in small screen. other fields added after will be maybe not well displayed. Maybe better to follow the classic rules "one field per line".

Nothing to do with your PR :
on runboat, I can not change data on partner form view (in PoS) and save. (changes are discarded).
Don't know why, but the bug is present also in runboat of 16.0 branch. so unrelated with your contribution. (but annoying !)

legalsylvain · 2023-11-28T20:19:44Z

pos_partner_location_abstract/models/res_config_settings.py

+class ResConfigSettings(models.TransientModel):
+    _inherit = "res.config.settings"
+
+    auto_create_map_data = fields.Boolean(


Question :
This fields looks to be not used (and not available in any view).
Did I missed something ?

legalsylvain · 2023-11-28T20:35:54Z

pos_partner_location_abstract/models/pos_config.py

+class PosConfig(models.Model):
+    _inherit = "pos.config"
+
+    geolocalize_tech_name = fields.Char(compute="_compute_geolocalize")


If I understand this field correctly, this should be a Selection field, don't you think ? (openstreetmap / googlemap). don't you think ?

If I understand this field correctly, this should be a Selection field, don't you think ? (openstreetmap / googlemap). don't you think ?

I thinks it's not better idea. This field is compute by Geo Provider configuration parameter. Provider we can update or add new what raise an error, that is why this field is just Char.

legalsylvain · 2023-11-28T20:36:13Z

pos_partner_location_abstract/models/pos_config.py

+    @api.model
+    def _set_extended_data(self):
+        return {}
+
+    def _set_pos_config_parameter(self, tech_name, ext_vals=None):
+        _ = ext_vals or {}
+        for config in self:
+            config.geolocalize_tech_name = tech_name


I don't understand very well why both functions are required.

I don't understand very well why both functions are required.

These functions helping fast and without problems configure new providers for POS.

legalsylvain · 2023-11-28T20:37:14Z

pos_partner_location_abstract/models/pos_session.py

+    _inherit = "pos.session"
+
+    def _loader_params_res_partner(self):
+        res = super(PosSession, self)._loader_params_res_partner()


nitpicking.

Suggested change

res = super(PosSession, self)._loader_params_res_partner()

res = super()._loader_params_res_partner()

em230418 · 2023-11-29T04:39:53Z

pos_partner_location_google_map/static/src/js/PartnerDetailsMapGoogleEdit.esm.js

+            ) {
+                return true;
+            }
+            return super.accessToMap;


Function call must be returned, not function itself.

Function call must be returned, not function itself.

This function is a property, that is why super.accessToMap returned property value.

em230418 · 2023-11-29T04:42:43Z

pos_partner_location_abstract/static/src/js/PartnerMapEdit.esm.js

+    }
+
+    onHandleMap() {
+        return false;


Returned value is not used. No need to return anything.

em230418 · 2023-11-29T04:56:53Z

pos_partner_location_google_map/static/src/js/PartnerMapGoogleEdit.esm.js

+                this.geocoder.geocode({location: latLng}, (results, status) => {
+                    if (status === google.maps.GeocoderStatus.OK) {
+                        this.getFormattedAddress(results[0].place_id);
+                        this.addrInput.el.value = results[0].formatted_address;


geocode returns formatted address in results[0].formatted_address and here we also do something to get formatted address using "getFormattedAddress".

this.address inside body getFormattedAddress could be set in 5 seconds if bad internet connection, meanwhile this.addrInput.el.value is set very quickly. Is it fine here? You are sure, we don't need to "await" for getFormattedAddress?

em230418 · 2023-11-29T04:58:29Z

pos_partner_location_google_map/static/src/js/PartnerMapGoogleEdit.esm.js

+        update_marker(lat, lng) {
+            super.update_marker(lat, lng);


Please use "camelCase", not "snake_case". Since in js snake_case naming for methods is not popular.

Thanks, I will change it.

em230418 · 2023-11-29T05:07:26Z

pos_partner_location_google_map/models/base_geocoder.py

+        status = google.query_addr({"place_id": place_id})
+        return google.get_result() if status else {}


On bad status, there should be warning indicated. At least in logs.

geomer198 · 2023-11-29T13:10:33Z

@legalsylvain @em230418 Could you check changes?

ivs-cetmix · 2023-12-10T17:46:50Z

pos_partner_location_abstract/readme/DESCRIPTION.rst

@@ -0,0 +1 @@
+This module is base for module pos_partner_location_google_map (Google Map).


This module implements base functionality for using different map providers in POS.
It allows to select partner location directly on a map and stores selected location in the res.partner model.
It also tries to evaluate partner's address from coordinates and provides a helper function that generates location QR code.
NB: This is a core module which is meant to be used in further customisations.

Description is updated

ivs-cetmix · 2023-12-10T17:47:19Z

pos_partner_location_abstract/models/res_partner.py

+
+    def _compute_qr_code_url(self):
+        """Compute QR Code URL for map provider"""
+        self.write({"qr_code_url": ""})


This is a non stored field. So using "write" will do no benefit here because no db tables are updated.
You should use "update" instead.

Agree with you, applied proposed changes.

DemchukM

Please add author and license information to all files.
Everything else looks good

ivs-cetmix

Finally LGTM

legalsylvain · 2023-12-16T18:21:41Z

@em230418, could you update your review ?

DemchukM

LGTM

ivs-cetmix · 2024-01-05T22:44:00Z

Hey @em230418 happy new year! Waiting for your updated review 😄

em230418 · 2024-01-07T08:20:32Z

pos_partner_location_abstract/static/src/js/PartnerDetailsMapEdit.esm.js

+                if (this.accessToMap) {
+                    await this.partnerMap();
+                } else {
+                    this.showPopup("ErrorPopup", {
+                        title: this.env._t("Map Error"),
+                        body: this.env._t("Cannot access map functions!"),
+                    });
+                }
+            } catch (e) {
+                if (
+                    identifyError(e) instanceof ConnectionLostError ||
+                    ConnectionAbortedError
+                ) {
+                    this.showPopup("OfflineErrorPopup", {
+                        title: this.env._t("Network Error"),
+                        body: this.env._t(
+                            "Cannot access product information screen if offline."
+                        ),
+                    });
+                } else {
+                    this.showPopup("ErrorPopup", {
+                        title: this.env._t("Unknown error"),
+                        body: this.env._t(
+                            "An unknown error prevents us from loading product information."
+                        ),
+                    });
+                }


Since we "await" from partnerMap, I suggest also to await "this.showPopup".

em230418 · 2024-01-07T08:23:37Z

pos_partner_location_abstract/tests/test_pos_session.py

+    def setUp(self):
+        super(TestPointOfSaleCommon, self).setUp()
+
+    def test_loader_params_res_partner(self):


Do we need to override setUp here?

em230418 · 2024-01-07T08:33:02Z

pos_partner_location_abstract/tests/test_pos_ui.py

+        partner = self.env["res.partner"].search([("id", "=", partner.id)])
+        self.assertEqual(partner.partner_latitude, 15.0)
+        self.assertFalse(partner.partner_longitude)


Why running "search" instead of validating fields?

geomer198 · 2024-01-07T17:48:07Z

@em230418 Could you check changes?

ivs-cetmix · 2024-04-01T23:17:38Z

Hi @legalsylvain , would appreciate you having a look at this when you have time. Thank you in advance!)

legalsylvain · 2024-04-02T09:41:18Z

Hi @legalsylvain , would appreciate you having a look at this when you have time. Thank you in advance!)

I can not test on runboat. But there are many approvals, so :

/ocabot merge nobump

OCA-git-bot · 2024-04-02T09:41:30Z

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 16.0-ocabot-merge-pr-1002-by-legalsylvain-bump-nobump, awaiting test results.

OCA-git-bot · 2024-04-02T09:52:20Z

Congratulations, your PR was merged at ae161b8. Thanks a lot for contributing to OCA. ❤️

geomer198 marked this pull request as draft May 20, 2023 20:13

geomer198 force-pushed the 16.0-pos_partner_location_map-add branch from 084c41c to 70eae27 Compare May 20, 2023 20:41

legalsylvain reviewed May 20, 2023

View reviewed changes

em230418 suggested changes May 20, 2023

View reviewed changes

ivs-cetmix suggested changes May 21, 2023

View reviewed changes

em230418 reviewed May 23, 2023

View reviewed changes

geomer198 marked this pull request as ready for review May 30, 2023 15:42

geomer198 marked this pull request as draft July 26, 2023 21:53

GabbasovDinar approved these changes Aug 8, 2023

View reviewed changes

geomer198 force-pushed the 16.0-pos_partner_location_map-add branch from 163f3c6 to 0605606 Compare August 8, 2023 20:49

geomer198 marked this pull request as ready for review August 8, 2023 20:50

ivs-cetmix approved these changes Aug 9, 2023

View reviewed changes

legalsylvain added this to the 16.0 milestone Sep 26, 2023

geomer198 force-pushed the 16.0-pos_partner_location_map-add branch from 0605606 to 1a44739 Compare November 28, 2023 17:48

legalsylvain reviewed Nov 28, 2023

View reviewed changes

em230418 suggested changes Nov 29, 2023

View reviewed changes

geomer198 force-pushed the 16.0-pos_partner_location_map-add branch 2 times, most recently from 039569b to 1601e38 Compare December 9, 2023 21:55

ivs-cetmix suggested changes Dec 10, 2023

View reviewed changes

DemchukM reviewed Dec 16, 2023

View reviewed changes

geomer198 force-pushed the 16.0-pos_partner_location_map-add branch from d7368bb to 6f28089 Compare December 16, 2023 17:11

ivs-cetmix approved these changes Dec 16, 2023

View reviewed changes

DemchukM approved these changes Dec 18, 2023

View reviewed changes

geomer198 force-pushed the 16.0-pos_partner_location_map-add branch 3 times, most recently from 1dab65c to 37c54c5 Compare December 28, 2023 15:07

em230418 suggested changes Jan 7, 2024

View reviewed changes

em230418 approved these changes Jan 29, 2024

View reviewed changes

OCA-git-bot added the approved label Jan 29, 2024

geomer198 added 2 commits February 21, 2024 17:08

[ADD] pos_partner_location_abstract: Module added.

21e79f2

[ADD] pos_partner_location_google_map: Module added.

0cbc999

geomer198 force-pushed the 16.0-pos_partner_location_map-add branch from 18d3684 to 0cbc999 Compare February 21, 2024 13:08

OCA-git-bot added the bot is merging ⏳ label Apr 2, 2024

OCA-git-bot merged commit a34eb1e into OCA:16.0 Apr 2, 2024
5 of 7 checks passed

OCA-git-bot added merged 🎉 and removed bot is merging ⏳ labels Apr 2, 2024

		@@ -0,0 +1,2 @@
		This module allows to select partner address directly on map.
		For this Google Maps are used.

	res = super(PosSession, self)._loader_params_res_partner()
	res = super()._loader_params_res_partner()

		status = google.query_addr({"place_id": place_id})
		return google.get_result() if status else {}

		@@ -0,0 +1 @@
		This module is base for module pos_partner_location_google_map (Google Map).

[16.0][ADD] pos_partner_location_map #1002

[16.0][ADD] pos_partner_location_map #1002

Conversation

geomer198 commented May 20, 2023

legalsylvain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

em230418 left a comment

Choose a reason for hiding this comment

ivs-cetmix commented May 23, 2023

geomer198 commented May 30, 2023

legalsylvain commented May 30, 2023 • edited Loading

geomer198 commented May 30, 2023

legalsylvain commented May 30, 2023

GabbasovDinar left a comment

Choose a reason for hiding this comment

ivs-cetmix left a comment

Choose a reason for hiding this comment

ivs-cetmix commented Aug 19, 2023

ivs-cetmix commented Nov 28, 2023

legalsylvain commented Nov 28, 2023

legalsylvain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geomer198 commented Nov 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DemchukM left a comment

Choose a reason for hiding this comment

ivs-cetmix left a comment

Choose a reason for hiding this comment

legalsylvain commented Dec 16, 2023

DemchukM left a comment

Choose a reason for hiding this comment

ivs-cetmix commented Jan 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geomer198 commented Jan 7, 2024

ivs-cetmix commented Apr 1, 2024

legalsylvain commented Apr 2, 2024

OCA-git-bot commented Apr 2, 2024

OCA-git-bot commented Apr 2, 2024

legalsylvain commented May 30, 2023 •

edited

Loading