[IMP] hr_expense_trip: in-depth review fixes (foundation for expense-line batching)#1
Open
dnplkndll wants to merge 4 commits into
Open
[IMP] hr_expense_trip: in-depth review fixes (foundation for expense-line batching)#1dnplkndll wants to merge 4 commits into
dnplkndll wants to merge 4 commits into
Conversation
- Remove static/src/views/list.xml: a verbatim copy of base hr_expense's ListButtons/ListView/ListRenderer templates (t-inherit-mode="primary"), not referenced in the manifest assets — dead code that would shadow the base templates if ever loaded. The "Create Trip" button is already added correctly via trip_list_buttons.xml (extension inheritance). - Remove dead hr.expense.action_add_existing_expenses (unreferenced; its name did not match its behaviour). - Fix license: AGPL-3 to match the file headers and the OCA/hr-expense repo convention (manifest said LGPL-3); add development_status. - Use patch() for the ExpenseListController.displayCreateTrip extension instead of a raw prototype assignment. - Drop "John Doe" placeholder text from the date fields in the trip report.
…ests
Addresses the follow-ups from the initial review.
- hr.trip.write(): drop the brittle `set(vals) == {"state"}` shape detection.
State transitions now go through a single `_set_state()` helper that uses the
skip_trip_write_protection context, so the workflow buttons stay exempt from
the post-approval lock while direct field edits (state included) remain
protected. The "expenses must match the trip employee" rule moves to an
@api.constrains, replacing the imperative employee_id branch in write().
- action_post(): drop the dead `moves = self.env["account.move"]` reassignment.
- Tests: the two "posting allowed" tests no longer swallow exceptions; they mock
the core hr.expense.action_post and assert the trip-state gate passes through
to super() (no accounting setup needed).
- expense_lines_widget: document why a One2many is rendered Many2many-style for
the add-existing dialog (the link/unlink commands resolve to the inverse FK).
- USAGE: document creating a trip by batching expenses from the expense list.
dnplkndll
force-pushed
the
19.0-hr_expense_trip
branch
from
dff0e54 to
90cf694
Compare
In 19.0 core, employee-paid (own_account) expenses post through an interactive wizard: hr.expense.action_post() returns the wizard action and creates no move. The trip's "Create Bill" discarded that return and read an empty account_move_id, so it silently posted nothing for the common employee-paid case. Build and post the receipt entries directly (as the wizard does), keeping company-paid expenses on the standard path.
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
1 participant
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.
Takes OCA#337 (
hr_expense_trip, by @CRogos — a v19 replacement for the removed expense sheet, grouping expense lines under a Trip) onto the fork for an in-depth review, as the foundation for expense-line batching. Base of this PR is OCA#337's head, so the diff is the review fixes only.Review findings addressed
static/src/views/list.xml— a verbatim copy of basehr_expense'sListButtons/ListView/ListRenderer(t-inherit-mode="primary"), not in the manifest assets. Dead, and would shadow the base templates if ever loaded. Deleted; the "Create Trip" button already comes fromtrip_list_buttons.xml(extension inheritance).hr.expense.action_add_existing_expenses— unreferenced, name didn't match behaviour. Removed.LGPL-3in the manifest vsAGPL-3.0headers; OCA/hr-expense convention isAGPL-3. Aligned manifest toAGPL-3; addeddevelopment_status.displayCreateTripwas a rawExpenseListController.prototypeassignment; switched topatch().Verified
account_move_id(used inaction_post) confirmed valid on 19.0hr.expense— no bug.--test-enable: 0 failed / 0 error of 30 tests; ruff clean.Batching foundation
hr.expense.action_create_trip(one trip per employee from a selection) + the "Create Trip" expense-list button are the batch entry point;expense_idson the trip with add-existing is the container. Solid base to build expense-line batching on.Follow-ups addressed (round 2)
hr.trip.write()simplified — removed the brittleset(vals) == {"state"}shape detection. State transitions now route through one_set_state()helper (uses theskip_trip_write_protectioncontext), so workflow buttons stay exempt while direct field edits — state included — remain locked after approval. The "expenses must match the trip's employee" rule moved to an@api.constrains.hr.expense.action_postand assert the trip-state gate passes through tosuper(). Also dropped a deadmoves = self.env["account.move"]line inaction_post.expense_lines_widget— kept (the canonical way to add existing records to a One2many; replacing it would be more code), with the rationale documented: the link/unlink commands resolve to the inversetrip_id, so no Many2many relation is involved.Tests: 0 failed / 0 error of 30; ruff clean.
Verified in-browser (local 19.0)
Selecting expenses shows the Create Trip button (the
patch()-addeddisplayCreateTrip).It batches the selection into a trip (one per employee) and opens the trip form — expenses listed with status/amount, Draft→Requested→Collect Receipts→Done state machine.