FOSFA-473: Add Functionality To Import Credit Note And Credit Note Allocations#274
FOSFA-473: Add Functionality To Import Credit Note And Credit Note Allocations#274shahrukh-compuco wants to merge 1 commit into
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request adds bulk-import functionality for Credit Notes and Credit Note Allocations to the CiviCRM financeextras extension. By exposing these entities through new APIv3 wrappers, users can now leverage the csvimport extension to load financial data that previously required manual entry or custom scripting. The implementation ensures that accounting entries are generated consistently with existing APIv4 logic, includes robust validation for data consistency, and handles grouping of line items based on external identifiers. Highlights
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a Credit Note Importer system, featuring a CSV row importer, APIv3/v4 entities, and allocation management. The implementation handles grouping CSV rows by external ID and manages the associated accounting entries. Review feedback identifies a critical race condition in the pre-import cleanup hook that could lead to data corruption in multi-user environments. Additionally, the feedback highlights several style guide violations regarding internationalization and mandatory guards for APIv4 results, as well as logic issues in date parsing for historical records.
c9dfade to
5280b6a
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a robust Credit Note importer system, featuring a CSV row importer, APIv3 endpoints for importing and allocation, and database migrations to support external ID tracking. The feedback highlights several critical and medium-severity issues, primarily focusing on missing is_array() guards for APIv4 ->first() results as required by the repository style guide. Addressing these is necessary to prevent fatal errors when records are missing. Additionally, a validation check for positive line unit prices should be added to the importer to maintain accounting integrity.
5280b6a to
1ac1d33
Compare
efc37e2 to
e6a3d92
Compare
e6a3d92 to
f61c566
Compare
Overview
Adds bulk-import support for Credit Notes and Credit Note Allocations (entities provided by
io.compuco.financeextras) via thenz.co.fuzion.csvimportextension. Until now both entities could only be created one at a time through the on-screen forms or APIv4 — there was no way to feed a CSV file in.Two new APIv3 entities are introduced so csvimport can pick them up automatically:
CreditNoteAllocation— a thin APIv3 wrapper over the existing BAO. One CSV row = one allocation.CreditNoteImporter— a fake APIv3 entity dedicated to credit-note imports. One CSV row = one credit-note line; rows that share acredit_note_external_idare grouped into a single credit note with multiple lines (mirroring the membershipextrasimporterapi pattern). All accounting entries (financial transaction, financial items, entity-financial-trxn links) are produced exactly the way the APIv4CreditNote.saveaction produces them.Includes pre-write validation, friendly date handling, automatic tax derivation from the financial type's Sales Tax Account, and a per-import cleanup of the grouping shadow table so it doesn't grow unbounded across imports.
Before
After
Credit Note allocations
CreditNoteAllocationin csvimport's Entity to Import dropdown.CreditNoteAllocation::allocateaction produces.type_idfield accepts either the numeric option value or, via a paralleltype_namecolumn, the option-value name (e.g.invoice).api.required).type_id/type_nameeither-or rule (the only PHP-level required check, since the two are mutually exclusive).datecolumn accepts any stringstrtotime()understands; empty or unmapped dates default to today (no more 1970-01-01 records).Credit Note imports (with line items)
CreditNoteImporterin csvimport's Entity to Import dropdown.credit_note_external_idare grouped into one credit note with that many line items.CreditNote.saveproduces for the same N items in one go: same credit-note record, same line records, same financial transaction (one per credit note), same financial items, same entity-financial-trxn linkage.CRM_Core_PseudoConstant::getTaxRates()— no need to repeat tax rates per row.strtotime()can't parse → clear "Could not parse date" error.Technical details
New entities
Both entities expose APIv3 (for csvimport) and APIv4 (so csvimport's reference-field discovery via
civicrm_api4($entity, 'getFields')doesn't 404).New / modified files
Bulk-import surface
api/v3/CreditNoteAllocation.php— APIv3 wrapper that delegates toCRM_Financeextras_BAO_CreditNoteAllocation::createWithAccountingEntries. Addstype_namepseudo-constant resolver, amount/contact validators sharing a single fetch of the credit note + contribution, and an epoch-aware date helper.api/v3/CreditNoteImporter.php— APIv3 entry point that delegates toCRM_Financeextras_CreditNoteImporter_CSVRowImporter. Spec exposes the CSV columns to csvimport's column-mapping UI.Civi/Api4/CreditNoteImporter.php— APIv4 wrapper extendingGeneric\DAOEntity(required socivicrm_api4('CreditNoteImporter', 'getFields', …)succeeds; without it csvimport surfaces "API (CreditNoteImporter, getFields) does not exist" on every row).Importer internals (CreditNoteImporter)
CRM/Financeextras/DAO/CreditNoteImporter.php— fake DAO;fields()drives csvimport's column-mapping UI.CRM/Financeextras/BAO/CreditNoteImporter.php— empty BAO for convention.CRM/Financeextras/CreditNoteImporter/CSVRowImporter.php— the row processor. First row in an external-id group callsCRM_Financeextras_BAO_CreditNote::createWithAccountingEntries(matches the APIv4 save path); subsequent rows reuse the same financial transaction, append a line viaCRM_Financeextras_BAO_CreditNoteLine::createWithAcountingEntries, and re-aggregate the credit note totals from the persisted lines so per-line rounding never drifts.CRM/Financeextras/CreditNoteImporter/Exception/InvalidRowException.php— typed exception for row-level failures.xml/schema/CRM/Financeextras/CreditNoteImporter.entityType.php— registers the entity name with CiviCRM.External-id mapping
credit_note_external_id→ internal credit note id) lives in a CiviCRM custom group attached toCreditNote, identical in pattern to membershipextrasimporterapi'scivicrm_value_*_ext_idtables. CiviCRM auto-creates the shadow tablecivicrm_value_credit_note_ext_id.xml/customGroups_install.xml— declares thecredit_note_external_idcustom group and field.Civi/Financeextras/Setup/Manage/CreditNoteCustomGroupExtensionManager.php— registers theCreditNoteentity in thecg_extend_objectsoption group so CiviCRM accepts the<extends>CreditNote</extends>declaration. Wired into the upgrader'spostInstall,enable,disable,uninstalllists.SELECT entity_id FROM civicrm_value_credit_note_ext_id/INSERT INTO …); APIv4 custom-field reads/writes do not reliably persist for extension-defined DAO entities, and using raw SQL matches what membershipextrasimporterapi does.Civi/Financeextras/Hook/BuildForm/CreditNoteImporterPreImportCleanup.php— wipes the shadow table when the user lands on csvimport's Preview form for our entity. Hooked intohook_civicrm_buildForm(nothook_civicrm_postProcess) becauseCRM_Import_Form_Preview::postProcess()callsrunAllInteractive()→CRM_Utils_System::redirect()and exits the request, sopostProcessHook()never fires.financeextras.php's existingfinanceextras_civicrm_buildFormhook dispatcher.Upgrader
CRM/Financeextras/Upgrader.php— addedupgrade_1007()that drops legacy artefacts from earlier iterations of this work (a standalone mapping table, a directexternal_idcolumn on the credit-note table) and then installs the cg_extend_objects entry + custom group via the shared manager.postInstallalso installs the custom group. The new manager is added to all four lifecycle methods (postInstall,enable,disable,uninstall).