[16.0][IMP] base_geoengine: Include edit button in Popup #409

anusriNPS · 2025-08-07T06:34:19Z

Added edit button in popup of record shown in geoengine view which allows user to edit records by opening new tab in browser window.

cparadis-impressfoods

LGTM!

quirino95 · 2025-09-04T15:58:57Z

base_geoengine/static/src/js/views/geoengine/geoengine_renderer/geoengine_renderer.xml

+             <div
+                t-if="isGeoengineAdmin"
+                id="popup-editor"
+                class="ol-popup-editor text-primary"
+                t-on-click="onEditButtonClicked"
+            />


I have some observations about this:

Is really GeoenginAdmin check needed? I think is more correct that the check should be done on the work order itself (user could be GeoengineAdmin but not allowed to edit a specific WO, how is this situation managed?);

When moving the cursor over the button, it doesn't give me the impression that it's clickable. Please could you improve this by adding a clickable attribute or changing the div into a button or something similar?

When clicking on edit button, the form view just opened is "full screen", hiding the upper navigation bar, and when going back, map view is reset;

suggestion: clicking the "open" button, i see the same form that I have by clicking on "edit" button. What if the open button opens form view in edit mode (depending of access rules of course)? In that case, "edit" button would become obsolete, but the interface should become more clear;

Re: the first point, this is a generic geoengine module, it could be used with any object, not just fieldservice. But in general, I agree with checking permissions on the object itself if possible and not requiring admin rights on the Geoengine app.

If it's too complicated, Odoo probably already checks permissions in other ways so I'd rather leave both buttons for everyone, at least as a first implementation.

Regarding edit button, icon is shown as clickable attribute when hovered around it. Edit button opens url in new tab with navigation bar and also restores history in geoengine view.

Regarding permission on object level:
a. Tried to hide the editbutton using ormservice and reading permission for record from backend during onwillstart() in geoengineRenderer. However, each record related information is attached in geoengineRenderer only when displayPopupRecord() method is called and geoenginerecord is mounted based on information passed from recordPanel template. Component is getting destroyed while trying to fetch specific record from geoengineRenderer setup. So, could not hide the button completely. This would be best option for this case but could not achieved.
b. Currently, if a model has a field named "can_write", if passed in info_box template from geoengine view definition, record is validated based on can_write field and allows user to edit the record. If not, user is notified that sufficient permissions are not available to edit the record. If info_box template does not have "can_write" field, all users is allowed to edit the record.(default behaviour).

Note: Without approach "b", by default even when user is allowed to edit the record, permissions are validated in backend and user is blocked in case the user does not have sufficient permissions.

IMO, I would like to stick not using approach "b" as already backend permissions are validated even without exposing new field in model for this purpose which seems to be overhead.

Please let me know your views.

Changes on the edit button now are ok.
About permissions, since now the form opens in a new tab, is ok for me to remove new checks added in this PR and stay with the "default" ones when opening the new tab.

Removed checking permission based on "can_write" field introduced from model and preserved default behaviour for permission check of geoengine views for edit button.

OCA-git-bot · 2025-09-24T14:39:57Z

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

HekkiMelody

Small code cleanup suggestion, otherwise seems ok.

HekkiMelody · 2025-09-25T08:29:16Z

base_geoengine/static/src/js/views/geoengine/geoengine_controller/geoengine_controller.esm.js

+        var web_base_url = window.origin;
+        var web_url = window.location.href;
+        var searchParams = new URLSearchParams(web_url.split("#")[1]);
+        var action_url =
+            web_base_url +
+            "/web#id=" +
+            String(resId) +
+            "&view_type=form&model=" +
+            resModel +
+            "&menu_id=" +
+            searchParams.get("menu_id");
+        this.actionService.doAction({
+            type: "ir.actions.act_url",
+            target: "_blank",
+            url: action_url,
+        });


chore: not at all a fan of building the URL manually, but if there's no other easy way to do it, that's fine.

I would suggest building the action_url in a less "manual" way, like in this module https://github.com/OCA/web/blob/16.0/web_widget_open_tab/static/src/js/open_tab_widget.esm.js#L12-L25

Also, please use const and let instead of var.

Updated as suggested

Added edit button in popup of record shown in geoengine view which allows user to edit values of record from geoengine view.

HekkiMelody

Code review, LGTM

github-actions · 2026-01-25T12:54:06Z

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

HekkiMelody · 2026-01-26T08:57:51Z

@OCA/geospatial-maintainers We've been using this successfully for a few months now. I would be grateful if you could review this. Thanks

legalsylvain · 2026-01-26T11:24:51Z

No review, but as it is validated by 3 people. merging.

/ocabot merge minor

thanks for your contribution.

OCA-git-bot · 2026-01-26T11:24:59Z

This PR looks fantastic, let's merge it!
Prepared branch 16.0-ocabot-merge-pr-409-by-legalsylvain-bump-minor, awaiting test results.

Signed-off-by legalsylvain

OCA-git-bot · 2026-01-26T11:26:42Z

@legalsylvain your merge command was aborted due to failed check(s), which you can inspect on this commit of 16.0-ocabot-merge-pr-409-by-legalsylvain-bump-minor.

After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red.

HekkiMelody · 2026-01-26T11:29:24Z

I see that the error is due to

2026-01-26 11:26:30,614 334 INFO odoo odoo.addons.base_geolocalize.models.base_geocoder: openstreetmap nominatim service called 
2026-01-26 11:26:30,615 334 WARNING odoo odoo.addons.base_geolocalize.models.base_geocoder: Request to openstreetmap failed.
Code: 403
Content: b'<html>\n<head>\n<title>Access blocked</title>\n</head>\n<body>\n<h1>Access blocked</h1>\n\n<p>You have been blocked because you have violated the\n<a href="https://operations.osmfoundation.org/policies/nominatim/">usage policy</a>\nof OSM\'s Nominatim geocoding service. Please be aware that OSM\'s resources are\nlimited and shared between many users. The usage policy is there to ensure that\nthe service remains usable for everybody.</p>\n\n<p>Please review the terms and make sure that your\nsoftware adheres to the terms. You should in particular verify that you have set a\n<b>custom HTTP referrer or HTTP user agent</b> that identifies your application, and\nthat you are not overusing the service with massive bulk requests.\nIf you do that then the block usually goes away after some hours of inactivity.</p>\n\n<p>Please be aware that we are providing this service as-is and without any\nguarantees. We cannot provide support. If you need that, please have a look\nat comercial alternatives (also listed on the usage policy page) or set up\nyour own instance of Nominatim.</p>\n</body>\n</head>\n' 
2026-01-26 11:26:30,617 334 INFO odoo odoo.addons.geoengine_base_geolocalize.tests.test_geoengine_partner: ====================================================================== 
2026-01-26 11:26:30,617 334 ERROR odoo odoo.addons.geoengine_base_geolocalize.tests.test_geoengine_partner: ERROR: TestGeoenginePartner.test_geo_localize
Traceback (most recent call last):
  File "/opt/odoo/addons/base_geolocalize/models/base_geocoder.py", line 95, in _call_openstreetmap
    result = response.json()
  File "/opt/odoo-venv/lib/python3.10/site-packages/requests/models.py", line 900, in json
    return complexjson.loads(self.text, **kwargs)
  File "/opt/odoo-venv/lib/python3.10/site-packages/simplejson/__init__.py", line 514, in loads
    return _default_decoder.decode(s)
  File "/opt/odoo-venv/lib/python3.10/site-packages/simplejson/decoder.py", line 386, in decode
    obj, end = self.raw_decode(s)
  File "/opt/odoo-venv/lib/python3.10/site-packages/simplejson/decoder.py", line 416, in raw_decode
    return self.scan_once(s, idx=_w(s, idx).end())
simplejson.errors.JSONDecodeError: Expecting value: line 1 column 1 (char 0)

we may be hitting openstreetmaps a bit too often? We may have to refactor the tests to not call OSM, or skip some tests in case of Access Denied

anusriNPS · 2026-01-26T11:31:17Z

I see that the error is due to

2026-01-26 11:26:30,614 334 INFO odoo odoo.addons.base_geolocalize.models.base_geocoder: openstreetmap nominatim service called 
2026-01-26 11:26:30,615 334 WARNING odoo odoo.addons.base_geolocalize.models.base_geocoder: Request to openstreetmap failed.
Code: 403
Content: b'<html>\n<head>\n<title>Access blocked</title>\n</head>\n<body>\n<h1>Access blocked</h1>\n\n<p>You have been blocked because you have violated the\n<a href="https://operations.osmfoundation.org/policies/nominatim/">usage policy</a>\nof OSM\'s Nominatim geocoding service. Please be aware that OSM\'s resources are\nlimited and shared between many users. The usage policy is there to ensure that\nthe service remains usable for everybody.</p>\n\n<p>Please review the terms and make sure that your\nsoftware adheres to the terms. You should in particular verify that you have set a\n<b>custom HTTP referrer or HTTP user agent</b> that identifies your application, and\nthat you are not overusing the service with massive bulk requests.\nIf you do that then the block usually goes away after some hours of inactivity.</p>\n\n<p>Please be aware that we are providing this service as-is and without any\nguarantees. We cannot provide support. If you need that, please have a look\nat comercial alternatives (also listed on the usage policy page) or set up\nyour own instance of Nominatim.</p>\n</body>\n</head>\n' 
2026-01-26 11:26:30,617 334 INFO odoo odoo.addons.geoengine_base_geolocalize.tests.test_geoengine_partner: ====================================================================== 
2026-01-26 11:26:30,617 334 ERROR odoo odoo.addons.geoengine_base_geolocalize.tests.test_geoengine_partner: ERROR: TestGeoenginePartner.test_geo_localize
Traceback (most recent call last):
  File "/opt/odoo/addons/base_geolocalize/models/base_geocoder.py", line 95, in _call_openstreetmap
    result = response.json()
  File "/opt/odoo-venv/lib/python3.10/site-packages/requests/models.py", line 900, in json
    return complexjson.loads(self.text, **kwargs)
  File "/opt/odoo-venv/lib/python3.10/site-packages/simplejson/__init__.py", line 514, in loads
    return _default_decoder.decode(s)
  File "/opt/odoo-venv/lib/python3.10/site-packages/simplejson/decoder.py", line 386, in decode
    obj, end = self.raw_decode(s)
  File "/opt/odoo-venv/lib/python3.10/site-packages/simplejson/decoder.py", line 416, in raw_decode
    return self.scan_once(s, idx=_w(s, idx).end())
simplejson.errors.JSONDecodeError: Expecting value: line 1 column 1 (char 0)

we may be hitting openstreetmaps a bit too often? We may have to refactor the tests to not call OSM, or skip some tests in case of Access Denied

Will check on the issue

…tMap Avoiding direct call to openstreetmap which results in Access denied (403) due to resource usage limits and hence mocking data for openstreetmap url as part of geoengine partner testcases.

quirino95

LGTM

Thank you for the fix!

HekkiMelody

Code review, LGTM for the test mock too.

Thank you!

HekkiMelody · 2026-01-27T11:35:08Z

@OCA/geospatial-maintainers we've fixed the issue with the flaky tests (even though they were out of scope for this PR) could you try the merge again? Thank you

We can forward port both changes to the newest versions too

legalsylvain · 2026-01-27T11:45:20Z

thanks for the contribution !

/ocabot merge minor

OCA-git-bot · 2026-01-27T11:45:28Z

This PR looks fantastic, let's merge it!
Prepared branch 16.0-ocabot-merge-pr-409-by-legalsylvain-bump-minor, awaiting test results.

OCA-git-bot · 2026-01-27T11:47:32Z

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

HekkiMelody · 2026-01-27T12:02:39Z

Thank you @legalsylvain !

anusriNPS marked this pull request as draft August 7, 2025 06:34

cparadis-impressfoods approved these changes Aug 25, 2025

View reviewed changes

anusriNPS force-pushed the 16.0-enhance-geoengine branch from 3df97af to fd809d6 Compare September 3, 2025 12:27

anusriNPS marked this pull request as ready for review September 3, 2025 12:28

anusriNPS force-pushed the 16.0-enhance-geoengine branch from fd809d6 to fae81fc Compare September 3, 2025 12:37

quirino95 reviewed Sep 4, 2025

View reviewed changes

anusriNPS force-pushed the 16.0-enhance-geoengine branch 2 times, most recently from 67c79d9 to 801c2f6 Compare September 24, 2025 10:02

quirino95 approved these changes Sep 24, 2025

View reviewed changes

OCA-git-bot added approved ready to merge labels Sep 24, 2025

HekkiMelody reviewed Sep 25, 2025

View reviewed changes

[16.0][IMP] base_geoengine: Include edit button in Popup

6eeb3f9

Added edit button in popup of record shown in geoengine view which allows user to edit values of record from geoengine view.

anusriNPS force-pushed the 16.0-enhance-geoengine branch from 801c2f6 to 6eeb3f9 Compare September 26, 2025 06:41

HekkiMelody approved these changes Sep 26, 2025

View reviewed changes

github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Jan 25, 2026

OCA-git-bot added a commit that referenced this pull request Jan 26, 2026

Merge PR #409 into 16.0

6e007bf

Signed-off-by legalsylvain

OCA-git-bot added the bot is merging ⏳ label Jan 26, 2026

OCA-git-bot removed the bot is merging ⏳ label Jan 26, 2026

anusriNPS force-pushed the 16.0-enhance-geoengine branch from f0b0368 to 6fc3314 Compare January 27, 2026 10:28

[16.0][IMP] geoengine_base_geolocalize: Using mock data for OpenStree…

d125f93

…tMap Avoiding direct call to openstreetmap which results in Access denied (403) due to resource usage limits and hence mocking data for openstreetmap url as part of geoengine partner testcases.

anusriNPS force-pushed the 16.0-enhance-geoengine branch from 6fc3314 to d125f93 Compare January 27, 2026 10:38

quirino95 approved these changes Jan 27, 2026

View reviewed changes

HekkiMelody approved these changes Jan 27, 2026

View reviewed changes

OCA-git-bot added the bot is merging ⏳ label Jan 27, 2026

OCA-git-bot merged commit 738ca62 into OCA:16.0 Jan 27, 2026
7 checks passed

OCA-git-bot added merged 🎉 and removed bot is merging ⏳ labels Jan 27, 2026

HekkiMelody deleted the 16.0-enhance-geoengine branch January 27, 2026 12:02

Uh oh!

[16.0][IMP] base_geoengine: Include edit button in Popup #409

[16.0][IMP] base_geoengine: Include edit button in Popup #409

Conversation

anusriNPS commented Aug 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cparadis-impressfoods left a comment

Choose a reason for hiding this comment

Uh oh!

quirino95 Sep 4, 2025

Choose a reason for hiding this comment

Uh oh!

HekkiMelody Sep 5, 2025

Choose a reason for hiding this comment

Uh oh!

anusriNPS Sep 24, 2025

Choose a reason for hiding this comment

Uh oh!

quirino95 Sep 24, 2025

Choose a reason for hiding this comment

Uh oh!

anusriNPS Sep 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OCA-git-bot commented Sep 24, 2025

Uh oh!

HekkiMelody left a comment

Choose a reason for hiding this comment

Uh oh!

HekkiMelody Sep 25, 2025

Choose a reason for hiding this comment

Uh oh!

anusriNPS Sep 26, 2025

Choose a reason for hiding this comment

Uh oh!

HekkiMelody left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jan 25, 2026

Uh oh!

HekkiMelody commented Jan 26, 2026

Uh oh!

legalsylvain commented Jan 26, 2026

Uh oh!

OCA-git-bot commented Jan 26, 2026

Uh oh!

OCA-git-bot commented Jan 26, 2026

Uh oh!

HekkiMelody commented Jan 26, 2026

Uh oh!

anusriNPS commented Jan 26, 2026

Uh oh!

quirino95 left a comment

Choose a reason for hiding this comment

Uh oh!

HekkiMelody left a comment

Choose a reason for hiding this comment

Uh oh!

HekkiMelody commented Jan 27, 2026

Uh oh!

legalsylvain commented Jan 27, 2026

Uh oh!

OCA-git-bot commented Jan 27, 2026

Uh oh!

Uh oh!

OCA-git-bot commented Jan 27, 2026

Uh oh!

HekkiMelody commented Jan 27, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

anusriNPS commented Aug 7, 2025 •

edited

Loading

anusriNPS Sep 24, 2025 •

edited

Loading