[16.0][IMP] fieldservice_geoengie: Fix vector Layer related Errors#1453
[16.0][IMP] fieldservice_geoengie: Fix vector Layer related Errors#1453
Conversation
anusriNPS
changed the title
anusriNPS
changed the title
anusriNPS
force-pushed
the
16.0-fix-geoview
branch
from
ce2da70 to
d73c57e
Compare
anusriNPS
force-pushed
the
16.0-fix-geoview
branch
from
d73c57e to
905d4ab
Compare
|
Notifying user to define selected field in geoengine view from attribute_field_id whose field is of supported type: Notifying user when selected field in "attribute_field_id" is of supported type but not suitable for interval/custom range. Example: "color" field which is of "integer" type in "fsm.location" but could not generate interval/custom range, if not suppressed JS error is seen. Interval range: For custom classification with field type such as "date", based on this PR it suppress JS error but view displayed for "custom" will not be the expected view. This is because user needs to use proper field for selected field type to display customized view. Below is the view when "request_early" is selected from "attribute_field_id" for "custom". 
|
anusriNPS
force-pushed
the
16.0-fix-geoview
branch
from
905d4ab to
be19652
Compare
| SUPPORTED_ATT = [ | ||
| "float", | ||
| "integer", | ||
| "integer_big", | ||
| "related", | ||
| "function", | ||
| "date", | ||
| "datetime", | ||
| "char", | ||
| "text", | ||
| "selection", | ||
| ] |
There was a problem hiding this comment.
chore (non-blocking): I see these elsewhere in the code as well, but I suspect they may have remained from previous versions, as I cannot find values of ttype matching some of these.
On a fresh odoo installation, opening a shell and inspecting env["ir.model.fields"]._fields["ttype"].selection I get the following fields
['binary', 'boolean', 'char', 'date', 'datetime', 'float', 'html', 'integer', 'json', 'many2many', 'many2one', 'many2one_reference', 'monetary', 'one2many', 'properties', 'properties_definition', 'reference', 'selection', 'text', 'serialized']
There was a problem hiding this comment.
env["ir.model.fields"]._fields["ttype"].selection - list all available ttype values.
fieldservice_geoengine supports only fields based on SUPPORTED_ATT and NUM_ATT which is used in domain to filter from ttype values.
ttype will not match the fields of supported_att and num_att variables and it should list all available types. ttype list seems right and this is my understanding. Please correct me, if my views differ from your thoughts.
There was a problem hiding this comment.
Sorry, I probably wasn't very clear. I didn't mean to imply that fields should be added to SUPPORTED_ATT, but rather that some may be removed (maybe?), like integer_big, related and function.
Either way, non-blocking.
| rec.geo_repr == "colored" | ||
| and ( | ||
| rec.classification != "unique" | ||
| and rec.classification != "custom" | ||
| ) |
There was a problem hiding this comment.
suggestion (non-blocking): readability
| rec.geo_repr == "colored" | |
| and ( | |
| rec.classification != "unique" | |
| and rec.classification != "custom" | |
| ) | |
| rec.geo_repr == "colored" | |
| and rec.classification not in ("unique", "custom") |
I would also add brackets for clarity
(rec.geo_repr == "colored"
and rec.classification not in ("unique", "custom"))
or ....There was a problem hiding this comment.
Updated as suggested
anusriNPS
force-pushed
the
16.0-fix-geoview
branch
from
be19652 to
3634d45
Compare
Notifying user to define selected attribute_field_id of supported type from vector layer in infoBox template to avoid JS error. Addressed JS Error when supported data type even defined in geoengine view as well. When a field from attribute_field_id values selected from vector layer is not suitable for "custom" classification, it is notified to the user to avoid JS Error.Similarly for interval classification is also taken care. Domain for attribute_field_id is filter based on classification selection in vector layer to avoid JS Error.
anusriNPS
force-pushed
the
16.0-fix-geoview
branch
from
3634d45 to
461ee9e
Compare
|
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. |
github-actions
bot
added
the
stale
|
|
|
Does it need OCA/geospatial#417 to be merged first ? |
yes, actually, it does |
github-actions
bot
removed
the
stale
marcos-mendez
left a comment
marcos-mendez
left a comment
There was a problem hiding this comment.
Automated Review -- Tests Failed
1. Root Cause
The test failure is caused by a database connection error during the Odoo startup, not directly related to the changes in the PR. However, the new oca_dependencies.txt file references a nonexistent or broken geospatial dependency, which likely leads to a failure in module loading or dependency resolution.
2. Suggested Fix
- File:
fieldservice_geoengine/oca_dependencies.txt - Issue: The dependency URL and commit hash are invalid or unreachable.
- Fix: Either:
- Correct the dependency URL or commit hash to a valid reference.
- Remove the dependency if it's not required for the current changes.
- Confirm that the referenced PR (
refs/pull/417/head) is stable and accessible.
Line: 1
Current:geospatial https://github.com/OCA/geospatial refs/pull/417/head 9825f67d128918294b67b74d324d1e6d3259399b
Suggestion: Update to a working commit or remove if unnecessary.
3. Additional Code Issues
a. Potential crash in JS code
- File:
fieldservice_geoengine/static/src/js/views/geoengine/geoengine_renderer/geoengine_renderer.esm.js - Issue: In
styleVectorLayermethod, thetry...catchblock silently swallows errors and returns early without logging or notifying the user. - Fix: Log the error or provide a more informative fallback.
Lines: 113–118
Suggestion: Addconsole.error(error)or a more user-facing error notification.
b. Inconsistent handling of attribute_field_id_domain
- File:
fieldservice_geoengine/models/vector_layer.py - Issue: The
_compute_attribute_field_id_domainmethod builds a domain based on conditions, but it does not validate thatrec.geo_field_id.model_id.modelis a valid model. - Fix: Add a check or error handling to ensure
geo_field_id.model_id.modelis valid before using it in a domain.
Lines: 48–56
Suggestion: Add atry-exceptor validation forrec.geo_field_id.model_id.model.
4. Test Improvements
a. Add test for oca_dependencies.txt
- Type:
SavepointCaseorTransactionCase - Test Case: Ensure the module loads without errors when
oca_dependencies.txtis present and valid. - Tags:
@odoo-test,@runboat-ready
b. Test attribute_field_id_domain computation
- Type:
TransactionCase - Test Case: Create a
GeoVectorLayerrecord and assert thatattribute_field_id_domainis correctly computed based ongeo_reprandclassification. - Tags:
@odoo-test,@geoengine
c. Test JS error handling in geoengine_renderer.esm.js
- Type:
SavepointCaseorHttpCase - Test Case: Simulate an invalid
label_textto ensure the JS code doesn't crash and logs appropriately. - Tags:
@odoo-test,@frontend,@geoengine
d. Test checkAttributeFieldUsage and getClass methods
- Type:
TransactionCaseorSavepointCase - Test Case: Test edge cases in
getClasslikenull,undefined, or invalid values. - Tags:
@odoo-test,@geoengine
Summary
- The test failure is likely due to a broken dependency, not the core logic.
- The JS code has a silent error handler that should be improved.
- The domain computation logic should include validation for model references.
- Add unit tests for domain logic, JS error handling, and frontend behavior.
⏰ PR Aging Alert
This PR by @anusriNPS has been open for 158 days (5 months).
Every ignored PR is a contributor who might not come back. Review time matters. (OCA Aging Report)
Reciprocal Review Request
Hi everyone! I found some test failures on this PR and left detailed feedback above. I am happy to discuss or help debug. In the meantime, if any of you get a chance, I would appreciate a look at my open PR(s):
My open PRs across OCA:
- server-tools#3554 [MIG] datetime_formatter: Migration to 18.0
- server-tools#3548 [18.0][MIG] base_kanban_stage: Migration to 18.0
- hr-attendance#262 [16.0][ADD] Hr_attendance_idsecure: iDSecure (ControliD) attendance integration
- stock-logistics-workflow#2276 [16.0][ADD] stock_move_line_devaluation
- stock-logistics-workflow#2275 [16.0][ADD] Stock move line analytic account
- stock-logistics-workflow#2268 [16.0][ADD] stock_move_line_picking_partner
- purchase-workflow#2694 [16.0][IMP]Purchase workflow added to review state & exception fix
Reviewing each other's work helps the whole community move forward. Thank you!
Environment via OCA Neural Reviewer: Minikube + K8s Job + oca-ci/py3.10-odoo16.0 | Odoo 16.0
Automated review by OCA Neural Reviewer + qwen3-coder:30b
5 participants
Depends on base_geoengine PR #417
Domain for "attribute_field_id" is filtered based on "classification" field selection in vector layer, which helps to avoid JS error seen as "fsm.order" and "fsm.location" related fields are not validated based on "NUM_ATT" types. base_geoengine_validation.
Notifying user to define selected field from attribute_field_id of supported type in geoengine view which avoids below JS error observed:
Notifying users when a field of supported type from attribute_field_id is not suitable to use for classification type "custom" which avoids below observed JS Error:
As fieldservice_geoengine omits validation for custom classification related to "type" check on field selected in "attribute_field_id", when user selects fields of type "date" with "custom", below JS error observed:
