From 0e1d61c5f7ba9815f23b2e065409a1d5f6aeee2d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cornelius=20Wei=C3=9F?= Date: Thu, 7 Nov 2024 19:04:23 +0100 Subject: [PATCH] fix(Sales): reversal of reversed docuemnts --- .../tine20/Sales/Document/ControllerTest.php | 4 +- tine20/Sales/Model/Document/Abstract.php | 73 ++++++++++++++----- .../Sales/Model/DocumentPosition/Abstract.php | 6 +- .../Sales/js/Document/CreateFollowUpAction.js | 18 +++-- 4 files changed, 74 insertions(+), 27 deletions(-) diff --git a/tests/tine20/Sales/Document/ControllerTest.php b/tests/tine20/Sales/Document/ControllerTest.php index 7f59fdf346..51dba3e5cb 100644 --- a/tests/tine20/Sales/Document/ControllerTest.php +++ b/tests/tine20/Sales/Document/ControllerTest.php @@ -594,7 +594,7 @@ public function testInvoiceNumbers() $expander->expand(new Tinebase_Record_RecordSet(Sales_Model_Document_Invoice::class, [$invoice])); $translate = Tinebase_Translation::getTranslation(Sales_Config::APP_NAME, - new Zend_Locale(Tinebase_Config::getInstance()->{Tinebase_Config::DEFAULT_LOCALE})); + new Zend_Locale($invoice->{Sales_Model_Document_Abstract::FLD_DOCUMENT_LANGUAGE})); $inTranslated = $translate->_('IN-'); $piTranslated = $translate->_('PI-'); @@ -633,7 +633,7 @@ public function testDeliveryNumbers() $expander->expand(new Tinebase_Record_RecordSet(Sales_Model_Document_Delivery::class, [$delivery])); $translate = Tinebase_Translation::getTranslation(Sales_Config::APP_NAME, - new Zend_Locale(Tinebase_Config::getInstance()->{Tinebase_Config::DEFAULT_LOCALE})); + new Zend_Locale($delivery->{Sales_Model_Document_Abstract::FLD_DOCUMENT_LANGUAGE})); $dnTranslated = $translate->_('DN-'); $pdTranslated = $translate->_('PD-'); diff --git a/tine20/Sales/Model/Document/Abstract.php b/tine20/Sales/Model/Document/Abstract.php index c68ea10024..4c37d40b91 100644 --- a/tine20/Sales/Model/Document/Abstract.php +++ b/tine20/Sales/Model/Document/Abstract.php @@ -541,6 +541,16 @@ public function isBooked(): bool ->{Sales_Model_Document_Status::FLD_BOOKED}); } + public static function getStatusField(): string + { + return static::$_statusField; + } + + public static function getStatusConfigKey(): string + { + return static::$_statusConfigKey; + } + protected function _getPositionClassName(string $class): string { static $positionClasses = []; @@ -563,26 +573,44 @@ public function transitionFrom(Sales_Model_Document_Transition $transition) $this->{self::FLD_PRECURSOR_DOCUMENTS} = new Tinebase_Record_RecordSet(Tinebase_Model_DynamicRecordWrapper::class, []); $this->{self::FLD_POSITIONS} = new Tinebase_Record_RecordSet($positionClass, []); - $isReversal = false; - /** @var Sales_Model_Document_TransitionSource $record */ - foreach ($transition->{Sales_Model_Document_Transition::FLD_SOURCE_DOCUMENTS} as $record) { - if (!$record->{Sales_Model_Document_TransitionSource::FLD_SOURCE_DOCUMENT}->isBooked()) { + if (($isReversal = (null !== $transition->{Sales_Model_Document_Transition::FLD_SOURCE_DOCUMENTS}->find(Sales_Model_Document_TransitionSource::FLD_IS_REVERSAL, true))) + && null !== $transition->{Sales_Model_Document_Transition::FLD_SOURCE_DOCUMENTS}->find(Sales_Model_Document_TransitionSource::FLD_IS_REVERSAL, false)) { + throw new Tinebase_Exception_UnexpectedValue('source documents must be either all reversals or not'); + } + + // since source documents might have different models, you can't expand all at once, you will have to "sort" them by model ... or do each individually + // check all source documents are booked + // check that either all source documents where reversals (status === reversal) or not + $sourcesAreReversals = null; // true means "Reversal of Reversal" -> FollowUp Document + $srcDocs = $transition->{Sales_Model_Document_Transition::FLD_SOURCE_DOCUMENTS}->{Sales_Model_Document_TransitionSource::FLD_SOURCE_DOCUMENT}; + array_walk($srcDocs, function (Sales_Model_Document_Abstract $doc) use(&$sourcesAreReversals): void { + if (!$doc->isBooked()) { throw new Tinebase_Exception_Record_Validation('source document is not booked'); } + $docReversed = $doc->{$doc::getStatusField()} === Sales_Config::getInstance()->{$doc::getStatusConfigKey()}->records->find(Sales_Model_Document_Status::FLD_REVERSAL, true)?->getId(); + if (null === $sourcesAreReversals) { + $sourcesAreReversals = $docReversed; + } elseif ($sourcesAreReversals !== $docReversed) { + throw new Tinebase_Exception_Record_Validation('source documents reversal status mixed'); + } - Tinebase_Record_Expander::expandRecord($record->{Sales_Model_Document_TransitionSource::FLD_SOURCE_DOCUMENT}); + Tinebase_Record_Expander::expandRecord($doc); + }); + if ($sourcesAreReversals && $isReversal) { + throw new Tinebase_Exception_Record_Validation('reversal of reversal are followups, thus is_reversal must be false'); + } + + /** @var Sales_Model_Document_TransitionSource $record */ + foreach ($transition->{Sales_Model_Document_Transition::FLD_SOURCE_DOCUMENTS} as $record) { $addedPositions = 0; - $isReversal = $isReversal || (bool)$record->{Sales_Model_Document_TransitionSource::FLD_IS_REVERSAL}; // if the positions for this document are not specified, we take all of them if (empty($record->{Sales_Model_Document_TransitionSource::FLD_SOURCE_POSITIONS}) || $record->{Sales_Model_Document_TransitionSource::FLD_SOURCE_POSITIONS}->count() === 0) { - //$record->{Sales_Model_Document_TransitionSource::FLD_SOURCE_POSITIONS} = // why? remove those two lines? - //new Tinebase_Record_RecordSet(Sales_Model_DocumentPosition_TransitionSource::class, []); - if ($record->{Sales_Model_Document_TransitionSource::FLD_IS_REVERSAL}) { + if ($isReversal) { $record->{Sales_Model_Document_TransitionSource::FLD_SOURCE_DOCUMENT} ->{Sales_Model_Document_Abstract::FLD_REVERSAL_STATUS} = Sales_Config::DOCUMENT_REVERSAL_STATUS_REVERSED; } @@ -597,16 +625,19 @@ public function transitionFrom(Sales_Model_Document_Transition $transition) $sourcePosition = new Sales_Model_DocumentPosition_TransitionSource([ Sales_Model_DocumentPosition_TransitionSource::FLD_SOURCE_DOCUMENT_POSITION => $position, Sales_Model_DocumentPosition_TransitionSource::FLD_SOURCE_DOCUMENT_POSITION_MODEL => get_class($position), - Sales_Model_DocumentPosition_TransitionSource::FLD_IS_REVERSAL => $record->{Sales_Model_Document_TransitionSource::FLD_IS_REVERSAL}, + Sales_Model_DocumentPosition_TransitionSource::FLD_IS_REVERSAL => $isReversal, ]); /** @var Sales_Model_DocumentPosition_Abstract $position */ $position = new $positionClass([], true); try { - $position->transitionFrom($sourcePosition); + $position->transitionFrom($sourcePosition, $sourcesAreReversals); $this->{self::FLD_POSITIONS}->addRecord($position); $position->{Sales_Model_DocumentPosition_Abstract::FLD_DOCUMENT_ID} = null; ++$addedPositions; } catch (Tinebase_Exception_Record_Validation $e) { + $e->setLogLevelMethod('info'); + $e->setLogToSentry(false); + Tinebase_Exception::log($e); } } @@ -618,6 +649,9 @@ public function transitionFrom(Sales_Model_Document_Transition $transition) ->{Sales_Model_DocumentPosition_TransitionSource::FLD_SOURCE_DOCUMENT_POSITION}->getID()))) { throw new Tinebase_Exception_UnexpectedValue('sourcePosition in transition not found in source document!'); } + if ((bool)$sourcePosition->{Sales_Model_DocumentPosition_TransitionSource::FLD_IS_REVERSAL} !== $isReversal) { + throw new Tinebase_Exception_UnexpectedValue('transition source position needs to have same is_reversal state as transition source document'); + } $sourcePosition->{Sales_Model_DocumentPosition_TransitionSource::FLD_SOURCE_DOCUMENT_POSITION} = $sPosition; /** now this is important! we need to reference the same object here, so it gets dirty and we can update it if required */ @@ -627,13 +661,12 @@ public function transitionFrom(Sales_Model_Document_Transition $transition) /** @var Sales_Model_DocumentPosition_Abstract $position */ $position = new $positionClass([], true); - $position->transitionFrom($sourcePosition); + $position->transitionFrom($sourcePosition, $sourcesAreReversals); $this->{self::FLD_POSITIONS}->addRecord($position); $position->{Sales_Model_DocumentPosition_Abstract::FLD_DOCUMENT_ID} = null; ++$addedPositions; - $isReversal = $isReversal || (bool)$sourcePosition->{Sales_Model_DocumentPosition_TransitionSource::FLD_IS_REVERSAL}; - if ($sourcePosition->{Sales_Model_DocumentPosition_TransitionSource::FLD_IS_REVERSAL} && $record->{Sales_Model_Document_TransitionSource::FLD_SOURCE_DOCUMENT} + if ($isReversal && $record->{Sales_Model_Document_TransitionSource::FLD_SOURCE_DOCUMENT} ->{Sales_Model_Document_Abstract::FLD_REVERSAL_STATUS} !== Sales_Config::DOCUMENT_REVERSAL_STATUS_REVERSED) { $record->{Sales_Model_Document_TransitionSource::FLD_SOURCE_DOCUMENT} ->{Sales_Model_Document_Abstract::FLD_REVERSAL_STATUS} = Sales_Config::DOCUMENT_REVERSAL_STATUS_PARTIALLY_REVERSED; @@ -687,10 +720,11 @@ public function transitionFrom(Sales_Model_Document_Transition $transition) } } + $translation = Tinebase_Translation::getTranslation(Sales_Config::APP_NAME, + new Zend_Locale($this->{self::FLD_DOCUMENT_LANGUAGE})); if ($isReversal) { - $translation = Tinebase_Translation::getTranslation(Sales_Config::APP_NAME, - new Zend_Locale($this->{self::FLD_DOCUMENT_LANGUAGE})); - $this->{self::FLD_DOCUMENT_TITLE} = $translation->_('Reversal') . ' ' . implode(', ', + $this->{self::FLD_DOCUMENT_TITLE} = + $translation->_('Reversal') . ' ' . implode(', ', array_reduce($transition->{Sales_Model_Document_Transition::FLD_SOURCE_DOCUMENTS}->{Sales_Model_Document_TransitionSource::FLD_SOURCE_DOCUMENT}, function($carry, $document) { array_push($carry, $document->{Sales_Model_Document_Abstract::FLD_DOCUMENT_NUMBER}); return $carry; @@ -704,8 +738,13 @@ public function transitionFrom(Sales_Model_Document_Transition $transition) throw new Tinebase_Exception_UnexpectedValue('reversal transitions need to to have same source and target document class'); } } + $this->{static::$_statusField} = Sales_Config::getInstance()->{static::$_statusConfigKey}->records->find(Sales_Model_Document_Status::FLD_REVERSAL, true)->getId(); } else { + if ($sourcesAreReversals) { + $this->{self::FLD_DOCUMENT_TITLE} = + preg_replace("/^{$translation->_('Reversal')}/", $translation->_('Followup'), $this->{self::FLD_DOCUMENT_TITLE}); + } $this->{static::$_statusField} = Sales_Config::getInstance()->{static::$_statusConfigKey}->default; } diff --git a/tine20/Sales/Model/DocumentPosition/Abstract.php b/tine20/Sales/Model/DocumentPosition/Abstract.php index 1a7ac6b9e1..0c6ed04f44 100644 --- a/tine20/Sales/Model/DocumentPosition/Abstract.php +++ b/tine20/Sales/Model/DocumentPosition/Abstract.php @@ -549,7 +549,7 @@ public function getLocalizedDiscountString(): string return ''; } - public function transitionFrom(Sales_Model_DocumentPosition_TransitionSource $transition) + public function transitionFrom(Sales_Model_DocumentPosition_TransitionSource $transition, bool $reversalOfReversal): void { $source = $transition->{Sales_Model_DocumentPosition_TransitionSource::FLD_SOURCE_DOCUMENT_POSITION}; foreach (static::getConfiguration()->fieldKeys as $property) { @@ -582,7 +582,7 @@ public function transitionFrom(Sales_Model_DocumentPosition_TransitionSource $tr return; } - if ($transition->{Sales_Model_DocumentPosition_TransitionSource::FLD_IS_REVERSAL}) { + if ($reversalOfReversal || $transition->{Sales_Model_DocumentPosition_TransitionSource::FLD_IS_REVERSAL}) { $this->{self::FLD_UNIT_PRICE} = 0 - $this->{self::FLD_UNIT_PRICE}; if (Sales_Config::INVOICE_DISCOUNT_SUM === $this->{self::FLD_POSITION_DISCOUNT_TYPE}) { $this->{self::FLD_POSITION_DISCOUNT_SUM} = 0 - $this->{self::FLD_POSITION_DISCOUNT_SUM}; @@ -613,7 +613,7 @@ public function transitionFrom(Sales_Model_DocumentPosition_TransitionSource $tr $this->{self::FLD_QUANTITY} = $this->{self::FLD_QUANTITY} - $existingQuantities; $this->computePrice(); - } elseif ($transition->{Sales_Model_DocumentPosition_TransitionSource::FLD_IS_REVERSAL}) { + } elseif ($reversalOfReversal || $transition->{Sales_Model_DocumentPosition_TransitionSource::FLD_IS_REVERSAL}) { $this->computePrice(); } } diff --git a/tine20/Sales/js/Document/CreateFollowUpAction.js b/tine20/Sales/js/Document/CreateFollowUpAction.js index e74eb661ab..c72954a8e6 100644 --- a/tine20/Sales/js/Document/CreateFollowUpAction.js +++ b/tine20/Sales/js/Document/CreateFollowUpAction.js @@ -39,6 +39,10 @@ Promise.all([Tine.Tinebase.appMgr.isInitialised('Sales'), const sharedTransitionFlag = `shared_${targetRecordClass.getMeta('recordName').toLowerCase()}` const recipientField = `${targetRecordClass.getMeta('recordName').toLowerCase()}_recipient_id` const supportsSharedTransition = sourceRecordClass.hasField(sharedTransitionFlag) + const statusFieldName = `${sourceType.toLowerCase()}_status` + const statusDef = Tine.Tinebase.widgets.keyfield.getDefinitionFromMC(sourceRecordClass, statusFieldName) + const reversedStatus = _.find(statusDef.records, { reversal: true }) + return new Ext.Action(Object.assign({ text: config.text || app.formatMessage('Create { targetRecordName }', { targetRecordName }), iconCls: `SalesDocument_${targetType} ${isReversal ? 'SalesDocument_Reversal' : ''}`, @@ -46,12 +50,15 @@ Promise.all([Tine.Tinebase.appMgr.isInitialised('Sales'), let enabled = records.length if (isReversal) { - // reversals are allowed for booked documents only + // reversals are allowed for booked, non fully reversed documents only const statusFieldName = `${sourceType.toLowerCase()}_status` const statusDef = Tine.Tinebase.widgets.keyfield.getDefinitionFromMC(sourceRecordClass, statusFieldName) enabled = records.reduce((enabled, record) => { - return enabled && _.find(statusDef.records, {id: record.get(statusFieldName) })?.booked + return enabled && record.get('reversal_status') !== 'reversed' && _.find(statusDef.records, {id: record.get(statusFieldName) })?.booked }, enabled) + // revere a mix of reversals and non reversals is not allowed + const status = _.uniq(_.map(records, `data.${statusFieldName}`)) + enabled = enabled && (status.length < 2 || _.indexOf(status, reversedStatus.id) < 0) } action.setDisabled(!enabled) @@ -64,20 +71,21 @@ Promise.all([Tine.Tinebase.appMgr.isInitialised('Sales'), const maskEl = cmp.findParentBy((c) => {return c instanceof Tine.widgets.dialog.EditDialog || c instanceof Tine.widgets.MainScreen }).getEl() const mask = new Ext.LoadMask(maskEl, { msg: app.formatMessage('Creating { targetRecordsName }', { targetRecordsName }) }) - const statusFieldName = `${sourceType.toLowerCase()}_status` - const statusDef = Tine.Tinebase.widgets.keyfield.getDefinitionFromMC(sourceRecordClass, statusFieldName) + const unbooked = selections.reduce((unbooked, record) => { record.noProxy = true // kill grid autoSave const status = record.get(statusFieldName) return unbooked.concat(statusDef.records.find((r) => { return r.id === status })?.booked ? [] : [record]) }, []) - if (_.filter(selections, (document) => { return document.get('reversal_status') !== 'notReversed' }).length) { + + if (_.filter(selections, (document) => { return document.get(statusFieldName) === reversedStatus.id }).length) { if (await Ext.MessageBox.confirm( app.formatMessage('Create new { targetRecordName }?', { targetRecordName: targetRecordClass.getRecordName() }), app.formatMessage('Reversal { sourceRecordsName } cannot be undone. If you continue, a new { targetRecordName } will be created as a positive document.', { sourceRecordsName, targetRecordName: targetRecordClass.getRecordName() }) ) !== 'yes') { return false } } + if (unbooked.length) { if (await Ext.MessageBox.confirm( app.formatMessage('Book unbooked { sourceRecordsName }', { sourceRecordsName }),