[16.0][IMP] cetmix_tower_server Fixes from 17.0#481
Merged
CetmixGitDrone merged 1 commit into16.0from Mar 27, 2026
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughDisabled demo scheduled tasks and applied targeted fixes across jet/request/template/waypoint/server/variable/log/YAML handling, security rule adjustments, and extensive test additions and updates. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cetmix_tower_server/models/cx_tower_variable.py (1)
515-529:⚠️ Potential issue | 🟠 MajorPreserve empty-string server-template values here too.
The rest of
_get_value()now treats onlyNoneas “missing”, but this branch still usesor. If a server-template override is"", it incorrectly falls back to the global value.🔧 Suggested fix
if server_template: - return server_template_value_char or global_value_char + return ( + server_template_value_char + if server_template_value_char is not None + else global_value_char + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cetmix_tower_server/models/cx_tower_variable.py` around lines 515 - 529, In _get_value(), the server-template branch currently uses a truthy "or" which treats an empty string as missing; change the logic to treat only None as missing by explicitly checking server_template_value_char for None (e.g., if server_template_value_char is not None return it, else return global_value_char) so that empty-string overrides are preserved; keep the same pattern used in the later "Jet" branch and update the return for the server_template branch accordingly.cetmix_tower_server/models/cx_tower_server.py (1)
2369-2385:⚠️ Potential issue | 🟠 Major
no_fallbackis a no-op in this override.The signature now advertises a server-local lookup mode, but this implementation always goes through
_get_variable_values_by_references(), so callers still receive fallback values even when they explicitly passno_fallback=True.🔧 Suggested fix
def get_variable_value(self, variable_reference, no_fallback=False): @@ """ self.ensure_one() + if no_fallback: + return super().get_variable_value( + variable_reference, no_fallback=no_fallback + ) variable = self.env["cx.tower.variable"].get_by_reference(variable_reference) if not variable: return None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cetmix_tower_server/models/cx_tower_server.py` around lines 2369 - 2385, get_variable_value currently ignores the no_fallback flag by always calling variable._get_variable_values_by_references which returns fallbacks; change get_variable_value (and its call to variable._get_variable_values_by_references) to respect no_fallback by propagating the flag (e.g. call variable._get_variable_values_by_references(variable_references=[variable_reference], server=self, no_fallback=no_fallback)), and if that helper does not accept a no_fallback parameter, add a branch in get_variable_value that when no_fallback is True performs a server-only lookup (read the server-specific value directly from the variable record or via a server-only helper) and returns None if no server-specific value exists, otherwise return the found value; reference functions: get_variable_value and _get_variable_values_by_references.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cetmix_tower_server/models/cx_tower_jet_request.py`:
- Around line 258-260: The finalizer currently does a TOCTOU by checking
self.jet_id and then calling self.jet_id.sudo().write({"served_jet_request_id":
False}) which can clear a newer attach; change it to perform an atomic
conditional update on the jet row (update WHERE id = <jet_id.id> AND
served_jet_request_id = <self.id>) or acquire a row lock on the Jet record
before clearing so you only clear when served_jet_request_id still equals this
request; update the code paths that reference jet_id and served_jet_request_id
to use the conditional UPDATE or locking variant instead of the two-step
check-then-write.
In `@cetmix_tower_server/models/cx_tower_jet_waypoint.py`:
- Around line 396-412: The target waypoint is being marked only as
is_destination=True before the previous waypoint begins an async leave, so when
the leave plan completes _plan_finished() looks for a waypoint in "arriving" and
never resumes the target; change the initial write so it sets both
is_destination=True and state="arriving" (i.e. self.write({"is_destination":
True, "state": "arriving"})) so _plan_finished() can find it and call _arrive();
apply the same adjustment to the analogous code around the other occurrence
referenced (lines ~567-568) and keep the existing rollback that sets
is_destination=False on immediate leave failure.
In `@cetmix_tower_server/models/cx_tower_jet.py`:
- Around line 903-908: The loop in cx_tower_jet.py currently skips "command"
logs but should explicitly guard for file logs for consistency and defensive
coding: replace the condition using jet_log.log_type == "command" with an
explicit check that jet_log.log_type == "file" and only call
jet_log.file_template_id.create_file(server=jet.server_id, jet=jet,
if_file_exists="skip") when that check passes; this aligns behavior with the
same explicit check used where file creation is handled elsewhere and references
the same symbols jet.server_log_ids, jet_log.log_type, and
file_template_id.create_file.
In `@cetmix_tower_server/models/cx_tower_plan_log.py`:
- Around line 352-388: The post-hook savepoint blocks swallow failures leaving
the plan marked finished while hook state changes are rolled back; update each
except handler (for waypoint_id._plan_finished,
jet_template_install_id._flight_plan_finished, jet_id._flight_plan_finished) to
persist the hook failure on the plan record (e.g. write an error field like
hook_failure_message/hook_failure_at and set plan_status='failed' or a failure
flag) and then re-raise the exception (or otherwise surface the error) so the
failure is recorded and not silently ignored.
In `@cetmix_tower_server/models/cx_tower_server.py`:
- Around line 1746-1797: The code currently converts any dependent-jet failure
into GENERAL_ERROR before calling log_record.finish, losing actionable child
statuses returned by jet._trigger_action (see dependent_jets loop and
_trigger_action) such as JET_DEPENDENCIES_NOT_SATISFIED; change the logic so
that when exactly one dependent jet produced a non-zero status (or an explicit
status is present in its result) you preserve that child's status (use that
status value for status) and keep its error message in error/response, while
only using GENERAL_ERROR when multiple dependent_jets failed or no specific
child status is available; update the branch that builds error_parts, status,
error and response (and the call to log_record.finish) to implement this
conditional preservation.
In `@cetmix_tower_server/tests/test_command.py`:
- Line 676: The test currently asserts a literal implementation artefact by
setting rendered_code_expected = "cd False && mkdir test-odoo-1"; update the
assertion to check for the intended fallback behavior instead of the exact "cd
False" string — for example, assert that the generated command uses a
safe/default directory token (like the current directory or a fallback path) or
that it omits a failing cd and still contains "mkdir test-odoo-1". Locate the
variable rendered_code_expected (in tests/test_command.py) and replace the
literal check with a behavior-level expectation (e.g., contains "mkdir
test-odoo-1" and either contains a valid path token or does not contain "cd
False") so the test verifies semantics rather than the implementation artifact.
In `@cetmix_tower_server/tests/test_jet_template.py`:
- Around line 1893-1914: The test currently derives expected from
_topological_sort_dependency_graph which can mask regressions; update
test_get_all_dependencies_consistency_with_build_graph to compute expected
without calling _topological_sort_dependency_graph by either hard-coding the
known reverse-topological list of template ids/templates for the Odoo fixture
(based on jet_template_odoo.id and the known graph) or by asserting explicit
relative-order constraints: call _build_dependency_graph() and for each edge (u
-> v) assert that dependencies.index(graph[v]["template"]) <
dependencies.index(graph[u]["template"]); keep using _get_all_dependencies() and
validate against the concrete/hard assertions instead of using
_topological_sort_dependency_graph().
In `@cetmix_tower_server/tests/test_jet.py`:
- Around line 24-28: The test name/docstring claims the explicit request is
marked "processing" before dispatch but the assertions only verify that
_serve_jet_request() was called; update the test to either (A) rename the
test/method and docstring to reflect that it only asserts _serve_jet_request()
invocation, or (B) add an explicit assertion that the explicit request object's
status (e.g., request.status or explicit_request.status) equals "processing" at
the point just before dispatch/transition handling and/or that the transition
dispatch code observes that state; locate the test method
test_on_is_available_explicit_request_marked_processing_before_dispatch and the
helper _serve_jet_request to implement the chosen fix consistently (also apply
the same alignment to the similar test around lines 54-65).
---
Outside diff comments:
In `@cetmix_tower_server/models/cx_tower_server.py`:
- Around line 2369-2385: get_variable_value currently ignores the no_fallback
flag by always calling variable._get_variable_values_by_references which returns
fallbacks; change get_variable_value (and its call to
variable._get_variable_values_by_references) to respect no_fallback by
propagating the flag (e.g. call
variable._get_variable_values_by_references(variable_references=[variable_reference],
server=self, no_fallback=no_fallback)), and if that helper does not accept a
no_fallback parameter, add a branch in get_variable_value that when no_fallback
is True performs a server-only lookup (read the server-specific value directly
from the variable record or via a server-only helper) and returns None if no
server-specific value exists, otherwise return the found value; reference
functions: get_variable_value and _get_variable_values_by_references.
In `@cetmix_tower_server/models/cx_tower_variable.py`:
- Around line 515-529: In _get_value(), the server-template branch currently
uses a truthy "or" which treats an empty string as missing; change the logic to
treat only None as missing by explicitly checking server_template_value_char for
None (e.g., if server_template_value_char is not None return it, else return
global_value_char) so that empty-string overrides are preserved; keep the same
pattern used in the later "Jet" branch and update the return for the
server_template branch accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: bdfdd676-ffd5-4569-b52c-e87800c15a5f
📒 Files selected for processing (27)
cetmix_tower_server/demo/demo_data.xmlcetmix_tower_server/models/cx_tower_file.pycetmix_tower_server/models/cx_tower_jet.pycetmix_tower_server/models/cx_tower_jet_request.pycetmix_tower_server/models/cx_tower_jet_template.pycetmix_tower_server/models/cx_tower_jet_template_dependency.pycetmix_tower_server/models/cx_tower_jet_waypoint.pycetmix_tower_server/models/cx_tower_metadata_mixin.pycetmix_tower_server/models/cx_tower_plan_log.pycetmix_tower_server/models/cx_tower_server.pycetmix_tower_server/models/cx_tower_server_log.pycetmix_tower_server/models/cx_tower_variable.pycetmix_tower_server/models/cx_tower_variable_value.pycetmix_tower_server/models/res_users.pycetmix_tower_server/security/cx_tower_jet_template_security.xmlcetmix_tower_server/tests/__init__.pycetmix_tower_server/tests/test_command.pycetmix_tower_server/tests/test_jet.pycetmix_tower_server/tests/test_jet_template.pycetmix_tower_server/tests/test_jet_template_install.pycetmix_tower_server/tests/test_jet_waypoint.pycetmix_tower_server/tests/test_server_jet_action_command.pycetmix_tower_yaml/models/cx_tower_command.pycetmix_tower_yaml/models/cx_tower_scheduled_task.pycetmix_tower_yaml/models/cx_tower_scheduled_task_cv.pycetmix_tower_yaml/tests/test_command.pycetmix_tower_yaml/tests/test_yaml_export_wizard.py
ivs-cetmix
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cetmix_tower_server/models/cx_tower_jet_waypoint.py`:
- Around line 650-661: The failure branch filters destination candidates by
"(w.is_destination or w.state == 'arriving')" which is asymmetric with the
success path that uses filtered("is_destination"); change the filter in the
failure branch to only select waypoints with is_destination and not the current
id (use the same predicate as the success path on
self.jet_id.waypoint_ids.filtered), then keep the subsequent
write({"is_destination": False}) and call to
_finalize_create_waypoint_command_log(success=False, error=...) unchanged so
destination handling is consistent.
In `@cetmix_tower_server/models/cx_tower_jet.py`:
- Around line 977-981: The _trigger_action() function currently returns None
when the early-exit condition self.state_id == target_state and from_transition
is true (and similar branches), violating the new {status, error} contract;
change that bare return to return a dict like {"status": "skipped", "error":
None} (or {"status":"ok","error":None} per your semantics) and update the other
early-return branches referenced (the same-state/restart branches within
_trigger_action around the from_transition checks) to also return the structured
result so all paths consistently return the {status, error} dict.
- Around line 894-910: The cloned server logs are being copied with
server_id=False so the new jet's logs lose their server binding; update the copy
step for self.server_log_ids -> jet.server_log_ids (the log.copy(...) calls) to
set server_id to the new jet's server (use jet.server_id) instead of False, and
ensure that after copying you still assign file-type logs a new file via
file_template_id.create_file(server=jet.server_id, jet=jet,
if_file_exists="skip") and that command-type logs also retain
server_id=jet.server_id so server-scoped lookups/access rules remain correct.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 08740b9d-63a6-44c3-875b-2d1c9c1b5271
📒 Files selected for processing (5)
cetmix_tower_server/models/cx_tower_jet.pycetmix_tower_server/models/cx_tower_jet_waypoint.pycetmix_tower_server/tests/test_jet.pycetmix_tower_server/tests/test_jet_template.pycetmix_tower_server/tests/test_jet_waypoint.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cetmix_tower_server/models/cx_tower_jet_waypoint.py`:
- Around line 177-206: The current check using
existing_destinations/existing_by_jet misses duplicates inside the same
destination_waypoints batch; add an intra-batch duplicate check before the
external lookup by iterating destination_waypoints and building a map (e.g.,
batch_by_jet) from waypoint.jet_id.id to the first waypoint, skipping any in
"leaving"/"deleting" state, and if a second waypoint for the same jet is found
raise the same ValidationError (use waypoint.name and jet=waypoint.jet_id.name)
so that duplicates within the create/write batch are rejected before consulting
existing_destinations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 24295c0c-bdc7-43b7-82c1-8e727957e76b
📒 Files selected for processing (2)
cetmix_tower_server/models/cx_tower_jet.pycetmix_tower_server/models/cx_tower_jet_waypoint.py
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cetmix_tower_server/tests/test_jet_waypoint.py (1)
923-930:⚠️ Potential issue | 🟡 MinorMake this test start from the real pre-arrival state.
Line 929 still seeds the destination as
arriving, so this test would pass even if_plan_finished()regressed to the old state-based lookup. Useready+is_destination=Truehere to exercise the new handoff.🧪 Suggested setup
- # Create destination waypoint in arriving state + # Create destination waypoint in ready state; _plan_finished() should + # move it to arriving/current via `is_destination`. destination_waypoint = self.JetWaypoint.create( { "name": "Destination Waypoint", "jet_id": self.jet_test.id, "waypoint_template_id": self.waypoint_template.id, "is_destination": True, - "state": "arriving", + "state": "ready", } )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cetmix_tower_server/tests/test_jet_waypoint.py` around lines 923 - 930, The test is seeding the destination waypoint with state "arriving" which masks regressions in _plan_finished(); update the setup that calls JetWaypoint.create to use state "ready" and keep is_destination=True so the test truly starts from the pre-arrival state and exercises the new handoff logic in _plan_finished(); locate the JetWaypoint.create call in the test and change the "state" value to "ready" while leaving "is_destination": True.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cetmix_tower_server/models/cx_tower_jet_waypoint.py`:
- Around line 452-455: Setting is_destination on the target before the source
finishes leaving allows unlink() to delete a ready waypoint and leave no
destination; instead, either delay marking the target with is_destination until
the source's leave completes (i.e., move the self.write({"is_destination":
True}) into the successful completion callback/transaction of the leave flow),
or harden unlink()/unlink logic to refuse removal/transition of waypoints where
is_destination is True (add an early check in unlink() to raise/abort if
record.is_destination and state == "ready"). Update the code paths that call
write({"is_destination": True}) and the unlink() implementation accordingly
(referencing is_destination, unlink(), and the write(...) call) so the
destination cannot be removed while a leave is in progress.
- Around line 456-470: The code can call self._arrive() twice because
previous_waypoint._leave() may synchronously trigger _plan_finished() which
already advanced the destination; to fix, only set state to "arriving" and call
self._arrive() if the current record wasn't already in "arriving" (i.e., check
self.state before writing) and ensure you call self._arrive() only when you
actually changed the state to "arriving" (e.g., check previous_state !=
"arriving" or verify the write changed state); update the block around
previous_waypoint._leave(), previous_waypoint.state and the subsequent
self.write(...) / self._arrive() calls to include that guard so _arrive() is not
invoked redundantly.
---
Outside diff comments:
In `@cetmix_tower_server/tests/test_jet_waypoint.py`:
- Around line 923-930: The test is seeding the destination waypoint with state
"arriving" which masks regressions in _plan_finished(); update the setup that
calls JetWaypoint.create to use state "ready" and keep is_destination=True so
the test truly starts from the pre-arrival state and exercises the new handoff
logic in _plan_finished(); locate the JetWaypoint.create call in the test and
change the "state" value to "ready" while leaving "is_destination": True.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 987a474b-a09d-48e0-bd51-9c4803a99f87
📒 Files selected for processing (2)
cetmix_tower_server/models/cx_tower_jet_waypoint.pycetmix_tower_server/tests/test_jet_waypoint.py
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cetmix_tower_server/tests/test_jet_waypoint.py (1)
915-922:⚠️ Potential issue | 🟡 MinorUse
readyhere so the test exercises the new promotion logic.The real flow keeps the destination waypoint
readywithis_destination=Trueuntil the leave succeeds. Starting this fixture inarrivingmeans the test still passes even if the code stops promoting the destination before_arrive(), so it misses the regression this change is meant to cover.Based on learnings, in
cetmix_tower_server/models/cx_tower_jet_waypoint.pytheleavingsuccess path usesfiltered("is_destination")becausefly_to()keeps the target waypointreadyuntil leave completion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cetmix_tower_server/tests/test_jet_waypoint.py` around lines 915 - 922, The test creates a destination waypoint in "arriving" state which bypasses the promotion logic; change the JetWaypoint.create call for the destination waypoint in this test to set "state": "ready" (keeping "is_destination": True and other fields) so the test exercises the promotion behavior tied to _arrive()/fly_to() and matches the leaving-success path in cx_tower_jet_waypoint.py that filters on "is_destination".
♻️ Duplicate comments (1)
cetmix_tower_server/models/cx_tower_jet_waypoint.py (1)
468-486:⚠️ Potential issue | 🟠 MajorActive destinations can still be deleted mid-flight.
After Line 470 the target waypoint can sit in
readywithis_destination=Trueuntil the source leave plan completes.unlink()still acceptsreadywaypoints, so a delete/delete-plan in that window can either remove the destination entirely or flip it todeletingwhile_plan_finished()still expectsfiltered("is_destination")to resume it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cetmix_tower_server/models/cx_tower_jet_waypoint.py` around lines 468 - 486, Currently the code sets is_destination on the new waypoint before the previous waypoint's leave completes, allowing unlink()/delete to act on a ready waypoint mid-transition; move the is_destination write so it is only set after the previous waypoint's leave has succeeded (i.e., after previous_waypoint._leave() returns without error and/or when you determine leaving completed immediately), and only then set self.write({"is_destination": True}) immediately before calling self.write({"state":"arriving"}) / self._arrive(); adjust the rollback path (where previous_waypoint.state == "error") to ensure is_destination is not set and keep _plan_finished()'s filtered("is_destination") invariant intact (use the existing methods _leave, _arrive, unlink, _plan_finished, state, is_destination to locate and update the logic).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@cetmix_tower_server/tests/test_jet_waypoint.py`:
- Around line 915-922: The test creates a destination waypoint in "arriving"
state which bypasses the promotion logic; change the JetWaypoint.create call for
the destination waypoint in this test to set "state": "ready" (keeping
"is_destination": True and other fields) so the test exercises the promotion
behavior tied to _arrive()/fly_to() and matches the leaving-success path in
cx_tower_jet_waypoint.py that filters on "is_destination".
---
Duplicate comments:
In `@cetmix_tower_server/models/cx_tower_jet_waypoint.py`:
- Around line 468-486: Currently the code sets is_destination on the new
waypoint before the previous waypoint's leave completes, allowing
unlink()/delete to act on a ready waypoint mid-transition; move the
is_destination write so it is only set after the previous waypoint's leave has
succeeded (i.e., after previous_waypoint._leave() returns without error and/or
when you determine leaving completed immediately), and only then set
self.write({"is_destination": True}) immediately before calling
self.write({"state":"arriving"}) / self._arrive(); adjust the rollback path
(where previous_waypoint.state == "error") to ensure is_destination is not set
and keep _plan_finished()'s filtered("is_destination") invariant intact (use the
existing methods _leave, _arrive, unlink, _plan_finished, state, is_destination
to locate and update the logic).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: bc688f7c-28c9-4bcd-aa27-fa8c13311281
📒 Files selected for processing (3)
cetmix_tower_server/models/cx_tower_jet.pycetmix_tower_server/models/cx_tower_jet_waypoint.pycetmix_tower_server/tests/test_jet_waypoint.py
ivs-cetmix
force-pushed
the
16.0-t5313-cetmix_tower_server-fix-backport_from_17
branch
2 times, most recently
from
6b62223 to
3be4382
Compare
tendil
reviewed
Mar 26, 2026
ivs-cetmix
force-pushed
the
16.0-t5313-cetmix_tower_server-fix-backport_from_17
branch
5 times, most recently
from
6ad2e22 to
043850b
Compare
Contributor
Author
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cetmix_tower_server/tests/test_jet_waypoint.py (1)
552-563:⚠️ Potential issue | 🟡 MinorUpdate the stale setup comment to match the new call path.
The comment says the waypoint is created with
is_destination=True, but now that flag is passed inprepare(is_destination=True).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cetmix_tower_server/tests/test_jet_waypoint.py` around lines 552 - 563, Update the stale test comment: change the comment that currently states the waypoint is created with is_destination=True to reflect the new call path where is_destination is passed to prepare(is_destination=True); locate the JetWaypoint.create(...) call and the subsequent waypoint.prepare(is_destination=True) invocation and reword the comment to indicate the waypoint is created in draft state and the destination flag is supplied when calling prepare.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cetmix_tower_server/models/cx_tower_jet_waypoint.py`:
- Around line 162-205: The constraint _check_is_destination currently watches
only "is_destination" and "jet_id" so changes to waypoint.state won't re-run it;
update the `@api.constrains` decorator on _check_is_destination to include "state"
(i.e., `@api.constrains`("is_destination", "jet_id", "state")) so the validation
runs when state is written, ensuring invalid-state checks and
duplicate-destination detection still occur on state transitions.
In `@cetmix_tower_server/models/cx_tower_jet.py`:
- Around line 891-911: Before replacing or unlinking server log records, remove
their associated file records to avoid orphaned files: in the clone flow (from
clone() calling jet_template.create_jet() and the block that assigns
jet.server_log_ids) first gather the existing jet.server_log_ids' file_id fields
and unlink those files (checking they exist) before reassigning or calling
unlink(); additionally add an unlink() override on the cx.tower.server.log model
to cascade-delete its file_id (or set ondelete='cascade' on the file_id
relation) so any future deletes also remove attached files. Ensure you reference
cx.tower.server.log.unlink, the file_id field, and the
clone()/jet_template.create_jet() assignment when making the changes.
In `@cetmix_tower_server/tests/test_jet_waypoint.py`:
- Around line 1971-1981: The test
test_unlink_destination_waypoint_no_raise_context_logs currently claims to
verify that an error is logged but only checks that the record still exists;
either add an explicit logging assertion or update the name/docstring to remove
the logging claim. To add logging verification, wrap the unlink call in
self.assertLogs(...) targeting the module logger used by the unlink logic (use
the same logger name used in the code path, e.g., the logger in the destination
waypoint module) and assert the expected error message appears in the captured
output; alternatively, rename the test and adjust the docstring to something
like "no raise + record skipped" to reflect the actual assertion if you prefer
not to assert logs. Ensure you reference the existing test function
test_unlink_destination_waypoint_no_raise_context_logs and the
waypoint.with_context(...).unlink() call when making the change.
- Around line 1311-1366: The test
test_fly_to_leave_failure_does_not_keep_destination_arriving should also assert
that destination_waypoint does not become "current" after a failed leave; after
the existing checks (after result and current_waypoint.state assertions) add an
assertion that destination_waypoint.state != "current" (and optionally re-check
destination_waypoint.is_destination is False) to ensure the destination is
neither left arriving nor promoted to current when the source leave fails.
In `@cetmix_tower_yaml/models/cx_tower_scheduled_task_cv.py`:
- Around line 19-35: In _post_process_yaml_dict_values, replace the plain truthy
check on the record returned by
env["cx.tower.variable.value"].browse(variable_value_id) with a defensive
existence check using .exists() (e.g., assign variable_value =
self.env["cx.tower.variable.value"].browse(variable_value_id).exists() and only
proceed to read name, variable_id, option_id, value_char if variable_value is
truthy); this ensures you don't rely on browse() returning a valid DB record and
prevents reading fields from a non-existent record.
---
Outside diff comments:
In `@cetmix_tower_server/tests/test_jet_waypoint.py`:
- Around line 552-563: Update the stale test comment: change the comment that
currently states the waypoint is created with is_destination=True to reflect the
new call path where is_destination is passed to prepare(is_destination=True);
locate the JetWaypoint.create(...) call and the subsequent
waypoint.prepare(is_destination=True) invocation and reword the comment to
indicate the waypoint is created in draft state and the destination flag is
supplied when calling prepare.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ab1f57b8-6f19-4bc8-8ddb-e149f63f17e9
📒 Files selected for processing (16)
cetmix_tower_server/models/cx_tower_jet.pycetmix_tower_server/models/cx_tower_jet_waypoint.pycetmix_tower_server/models/cx_tower_jet_waypoint_template.pycetmix_tower_server/tests/test_jet.pycetmix_tower_server/tests/test_jet_template.pycetmix_tower_server/tests/test_jet_waypoint.pycetmix_tower_yaml/models/__init__.pycetmix_tower_yaml/models/cx_tower_command.pycetmix_tower_yaml/models/cx_tower_jet_template.pycetmix_tower_yaml/models/cx_tower_jet_waypoint_template.pycetmix_tower_yaml/models/cx_tower_scheduled_task.pycetmix_tower_yaml/models/cx_tower_scheduled_task_cv.pycetmix_tower_yaml/models/cx_tower_yaml_mixin.pycetmix_tower_yaml/tests/test_tower_yaml_mixin.pycetmix_tower_yaml/tests/test_yaml_import_wizard.pycetmix_tower_yaml/wizards/cx_tower_yaml_import_wiz.py
💤 Files with no reviewable changes (1)
- cetmix_tower_server/models/cx_tower_jet_waypoint_template.py
Contributor
Author
|
@CodeRabbit review commit 043850b |
|
✅ Actions performedReview triggered.
|
ivs-cetmix
force-pushed
the
16.0-t5313-cetmix_tower_server-fix-backport_from_17
branch
from
043850b to
761e63e
Compare
Demo and small fixes:
- Demo scheduled tasks are set to `active=False` so they do not run
unexpectedly on install.
- `cx_tower_file`: `server_dir` normalisation handles `None` safely
(`(values.get("server_dir") or "").strip()`).
- `cx_tower_jet`: Help text typo (“set by by” → “set by”).
Jets:
- Jet creation from template: Scheduled tasks assignment is
dedented so it runs for every created jet (not only inside a conditional branch).
- `clone_jet`: After `create_jet`, scheduled tasks are copied
from the source jet; server logs are reassigned (copy with `jet_id`
/`server_id` cleared, recreate files for non-command logs, or unlink
new-jet logs if the source had none). Avoids passing these through
`kwargs` in the old way.
- `_trigger_action`: Return on “not available” is aligned with
the surrounding control flow so the error path is actually returned.
- `_on_is_available`: Explicit and implicit transitions go through
`_serve_jet_request` (with `return` after dispatch) so requests
are tracked and finalized correctly.
Implicit template requests are filtered with `("jet_id", "=", False)`
so they do not steal jets that already have a target.
- `_command_runner_jet_action`: Aggregates errors from dependent jets
into a single message, and returns a main message if no errors occurred.
- `_trigger_action` returns a dictionary with the following keys:
- `status`: The status of the action
- `error`: The error message if the action is not available
Jet requests:
- `ensure_one()` when resolving `jet` / `jet_template` from arguments.
- `_finalize_jet_request`: Clears `served_jet_request_id` only
when it still points to this request,
using `sudo().write` to avoid access edge cases.
Jet template dependencies:
- `_get_all_dependencies`: Replaced level-only ordering with
Kahn topological sort (heap + id tie-break), reverse of topo
order for install line ordering; level-based fallback if the graph
has a cycle, with a warning.
- `cx_tower_jet_template_dependency`: TODO note that building the
full graph via `search([])` is inefficient.
Waypoints (`fly_to` / plan completion):
- `fly_to`: Destination is marked with `is_destination` first;
`arriving` is set only after leave succeeds (sync path) or in
`_plan_finished` for async leave; rollback and finalize create
log as failed if leave fails immediately.
- `_plan_finished`: On successful leave, destination can be found
via `is_destination` or `arriving`; on failed leave plan,
pending destination is cleared and its create log is finalized as failed.
- Conflict log: Includes the other waypoint’s name in the translation kwargs.
Plan log robustness:
- `cx_tower_plan_log`: Waypoint, template install, and jet post-finish hooks
run inside `savepoint()` with try/except and warning logs
so a failing callback does not abort the whole transaction.
Server / variables / users / security:
- `cx_tower_server`: Validates that a given jet belongs to the server
before proceeding; jet action path resolves `jet_action_id`
once and fails cleanly if missing; dependent jets use that cached action;
`get_variable_value` gains optional `no_fallback`
(signature only in this diff snippet—implementation may extend below).
- `cx_tower_server_log`: `jet_id` and `file_id` get
`copy=False` so clones do not duplicate bad links.
- `cx_tower_variable`: Value resolution uses `is None` instead
of truthiness so empty string is preserved through the fallback chain.
- `cx_tower_variable_value`: `_compute_is_global` depends on jet
fields; uniqueness constraint includes `variable_id`
- `res_users`: Tower access helper uses plain `if` / final
return False` and documents False as no access.
- `cx_tower_jet_template_security.xml`: Managers of related jets
(`jet_ids.manager_ids`) can see templates under the same OR rules as before.
Metadata mixin:
- `update_metadata` (or equivalent) returns `True` after write
(callers expecting a boolean).
Task 5309
ivs-cetmix
force-pushed
the
16.0-t5313-cetmix_tower_server-fix-backport_from_17
branch
from
761e63e to
5b377dd
Compare
ivs-cetmix
changed the title
Contributor
Author
|
/ocabot merge patch |
|
What a great day to merge this nice PR. Let's do it! |
|
Congratulations, your PR was merged at 4c12e40. Thanks a lot for contributing to cetmix. ❤️ |
CetmixGitDrone
deleted the
16.0-t5313-cetmix_tower_server-fix-backport_from_17
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Backport fixes from #480
Task 5313
Summary by CodeRabbit
Bug Fixes
New Features
Security/Permissions
Tests
Changes