[18.0][ADD][WIP] project_revenue_recognition#1673
Conversation
e5a251f to
04d421c
Compare
a03b461 to
3f83ea8
Compare
alexey-pelykh
left a comment
There was a problem hiding this comment.
WIP / Draft PR -- Preliminary Observations
This PR is marked as WIP / Draft with open TODO items (test script, readme), so this is not a full review. Sharing preliminary observations to help guide the remaining work.
Correctness & Logic
-
_get_amount_sale_invoicefilters only non-closed tasks (is_closed = False), meaning completed tasks are excluded from the sale order item fetch. This seems inverted for revenue recognition -- you likely want to include all tasks (or specifically closed ones) to compute total project value and progress. Please verify the intent. -
_compute_amount_diffonly handlesinvoice_projectcomparison but has noelsebranch -- ifcompare_withis falsy or a future selection value,amount_diffis never set (it will keep its previous stored value or default). Consider adding a fallbackrec.amount_diff = 0.0. -
action_adjust_costusesself.auto_poston the recordset level, but this is iteratingselfvia_create_moves. If multiple records are processed,self.auto_postwill raise a singleton error. Should be handled per-record. -
action_reverse_costreturnsaction_reverse()result from the wizard (a dict), but then the caller flow expects to also callself.action_reverse()to set state toreverse. The current flow opens the reversal wizard but never sets the recognition state toreverseuntil the wizard completes viaaccount_move_reversal.py. This coupling is fragile -- if the user cancels the wizard, the state never transitions. -
_get_account_mappingsilently returns(False, False)whenamount_diff == 0because theelsebranch handles negative-or-zero. When diff is exactly zero, journal entries with zero amounts will be created. Consider skipping JE creation when diff is zero. -
project_project.pyinheritsproject.projectbut adds no fields or methods. This is a no-op file and should either be removed or populated with the intended functionality.
Security
-
.sudo()usage in_get_amount_sale_invoice-- thesudo()call on fetched sale order items bypasses access control. This is common in Odoo for cross-model aggregation, but the scope should be documented (comment explaining why sudo is needed). -
Record rules look correct -- multi-company rules on both models are properly defined with global scope.
Odoo Best Practices
-
Missing
tracking=Trueon thestatefield -- since the model inheritsmail.thread, state changes should be tracked in the chatter. -
web_widget_x2many_2d_matrixdependency is declared in__manifest__.pybut never used in any view. This dependency should be removed unless there are planned views that use it. -
No
_check_companyconstraints --journal_idand the account fields onres.companyhave no company consistency checks. Consider addingcheck_company=Trueon relational fields that should respect multi-company boundaries. -
Wizard
reverse_movesassumes singleton onproject_revenue(project_revenue.id,project_revenue.action_reverse()) butmapped()can return multiple records. Addensure_one()or handle multi-record case.
Missing Pieces (per TODO)
- No tests -- the PR description confirms this is pending.
readme/USAGE.mdis empty -- the README Usage section renders blank.- No
ROADMAP.mdorHISTORY.rstfor planned improvements.
Manifest
"development_status"key is missing from__manifest__.py. The README badge says Beta, but the manifest should declare"development_status": "Beta"(or"Alpha"given WIP state).
Minor / Style
- The method
action_adjust_costand button label say "Cost" but this module is about "Revenue" recognition. Consider aligning terminology. readme/DESCRIPTION.mdis missing a trailing newline.
Happy to do a full review once the PR is out of draft with tests and documentation in place. Keep up the good work!
Review posted via CorporateHub OCA review campaign
Concept:
Auto-create Journal Entries to adjust revenue based on the difference between Project Progress and Posted Invoices.
TODO: