[16.0][IMP] helpdesk_mgmt_fieldservice: review fsm_order_close_wizard security

[SirPyTech]

Proposing again a fix that was already proposed for 14.0 in #593 and #404, hoping the third time's the charm 🍀
I also added a test 🚀

---

This commit reviews the security rules around the fsm_order_close_wizard.

The "Complete" button on the fsm.order triggered an access error for fieldservice.group_fsm_user_own. The commit allows fieldservice.group_fsm_user_own to use the wizard as well. However, the wizard is now only shown if the user also has write permission on the specific ticket (so as to play nice with a variety of setups, including helpdesk_mgmt.group_helpdesk_user_own).

If the user has permission to write on the ticket, the wizard will be shown as before; otherwise it will simply be skipped, but the fsm.order will be closed as it should.

There's also some code cleanup on the wizard, such as removing the unused team_id field.

[quirino95]

LGTM

[HekkiMelody]

Code review, LGTM.

Thanks for cleaning up the code and adding a test.

Since they're not being used, you could optionally remove the fields team_id and stage_id from the fsm.order.close.wizard model.

[OCA-git-bot]

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). 🤖

[SirPyTech]

Thanks for having a look!

Since they're not being used, you could optionally remove the fields team_id and stage_id from the fsm.order.close.wizard model.

stage_id is being used in

helpdesk/helpdesk_mgmt_fieldservice/wizards/fsm_order_close_wizard.py

Line 21 in b79432e

{"resolution": record.resolution, "stage_id": record.stage_id.id}

, note that it is still shown in the wizard just below the comment removed by this PR.

I have removed team_id.

[HekkiMelody]

Code review, LGTM

[marcos-mendez]

Automated Review -- Tests Failed

1. Root Cause

The test failure occurs because the database connection fails during the Odoo startup, likely due to an incorrect or missing database configuration in the test environment (e.g., runboat infrastructure). This is not caused by the code changes themselves but by the test runner setup.

---

2. Suggested Fix

There is no code-level fix needed for the database connection error. However, to ensure robustness and clarity:

• File: helpdesk_mgmt_fieldservice/models/fsm_order.py
  • Method: _check_tickets_access()
  • Issue: The method raises an AccessError and catches it, which is valid, but the logic can be simplified.
  • Suggested improvement: Replace the try/except with tickets.check_access_rights("write", raise_exception=False) to avoid exception handling overhead and improve readability.

def _check_tickets_access(self):
    """The current user can access the tickets of `self`."""
    return self.ticket_id.check_access_rights("write", raise_exception=False)

---

3. Additional Code Issues

• File: helpdesk_mgmt_fieldservice/wizards/fsm_order_close_wizard.py

  • Line: Removed team_id field
  • Issue: If team_id was used elsewhere in the wizard logic or view, removing it without ensuring no dependencies may break functionality.
  • Suggestion: Verify that team_id is not referenced in any computed fields, domain filters, or other logic in the wizard.

• File: helpdesk_mgmt_fieldservice/wizards/fsm_order_close_wizard.xml

  • Issue: Removed team_id and stage_id from the view
  • Suggestion: If stage_id is still required for validation or filtering, ensure it is still present in the view or re-added with proper domain logic if needed.

---

4. Test Improvements

To better cover the new access control logic and ensure extensibility:

Add Test Cases:

1. Test access control for different user groups:

   • Create a user in fieldservice.group_fsm_user and verify they can close orders.
   • Create a user in fieldservice.group_fsm_user_own and verify they can only close orders linked to their own tickets.

2. Test _check_tickets_access() method directly:

   • Use self.env["fsm.order"].with_user(user) to simulate access checks.
   • Test both positive and negative access scenarios.

3. Test with SavepointCase (OCA pattern):

   • Use SavepointCase to isolate tests and avoid side effects from access rights or data changes.

Example Test Snippet:

def test_check_tickets_access(self):
    user = self.own_doc_user
    order = self._create_ticket_fsm_orders(self.ticket_1)
    result = order.with_user(user)._check_tickets_access()
    self.assertTrue(result)

---

Summary

• The test failure is due to an infrastructure issue, not code.
• Improve _check_tickets_access() to avoid unnecessary exception handling.
• Ensure team_id and stage_id in wizard are still required or handled properly.
• Add tests to validate access control logic and use SavepointCase for better isolation.

---

⏰ This PR has been open for 61 days.
💤 Last activity was 52 days ago.

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