From 663ae217b0ba570026ed4f37e37d8702d7dd4dfc Mon Sep 17 00:00:00 2001 From: Andrei Shapiro Date: Fri, 14 Feb 2025 13:28:23 +0300 Subject: [PATCH 1/2] fix: keep the same unique QTI identifier even if unique ID feature flag disabled --- .../Service/TranslationUniqueIdSetter.php | 18 +-- .../Service/TranslationUniqueIdSetterTest.php | 106 ++++++++++++------ 2 files changed, 81 insertions(+), 43 deletions(-) diff --git a/models/classes/Translation/Service/TranslationUniqueIdSetter.php b/models/classes/Translation/Service/TranslationUniqueIdSetter.php index 1bc5267ae2..b228c2ef72 100644 --- a/models/classes/Translation/Service/TranslationUniqueIdSetter.php +++ b/models/classes/Translation/Service/TranslationUniqueIdSetter.php @@ -15,7 +15,7 @@ * along with this program; if not, write to the Free Software * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. * - * Copyright (c) 2024 (original work) Open Assessment Technologies SA. + * Copyright (c) 2024-2025 (original work) Open Assessment Technologies SA. */ declare(strict_types=1); @@ -59,24 +59,24 @@ public function addQtiIdentifierSetter(AbstractQtiIdentifierSetter $qtiIdentifie public function __invoke(core_kernel_classes_Resource $resource): void { - if ( - !$this->featureFlagChecker->isEnabled('FEATURE_FLAG_UNIQUE_NUMERIC_QTI_IDENTIFIER') - || !$this->featureFlagChecker->isEnabled('FEATURE_FLAG_TRANSLATION_ENABLED') - ) { + if (!$this->featureFlagChecker->isEnabled('FEATURE_FLAG_TRANSLATION_ENABLED')) { return; } $originalResource = $this->getOriginalResource($resource); $uniqueIdentifier = $this->getUniqueId($originalResource); - $resource->editPropertyValues( - $this->getProperty(TaoOntology::PROPERTY_UNIQUE_IDENTIFIER), - $uniqueIdentifier - ); $this->getQtiIdentifierSetter($resource)->set([ AbstractQtiIdentifierSetter::OPTION_RESOURCE => $resource, AbstractQtiIdentifierSetter::OPTION_IDENTIFIER => $uniqueIdentifier, ]); + + if ($this->featureFlagChecker->isEnabled('FEATURE_FLAG_UNIQUE_NUMERIC_QTI_IDENTIFIER')) { + $resource->editPropertyValues( + $this->getProperty(TaoOntology::PROPERTY_UNIQUE_IDENTIFIER), + $uniqueIdentifier + ); + } } private function getOriginalResource(core_kernel_classes_Resource $resource): core_kernel_classes_Resource diff --git a/test/unit/models/classes/Translation/Service/TranslationUniqueIdSetterTest.php b/test/unit/models/classes/Translation/Service/TranslationUniqueIdSetterTest.php index 9846e942ca..9954f02b9d 100644 --- a/test/unit/models/classes/Translation/Service/TranslationUniqueIdSetterTest.php +++ b/test/unit/models/classes/Translation/Service/TranslationUniqueIdSetterTest.php @@ -15,7 +15,7 @@ * along with this program; if not, write to the Free Software * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. * - * Copyright (c) 2024 (original work) Open Assessment Technologies SA. + * Copyright (c) 2024-2025 (original work) Open Assessment Technologies SA. */ declare(strict_types=1); @@ -65,18 +65,68 @@ protected function setUp(): void public function testUniqueIdFeatureDisabled(): void { $this->featureFlagChecker - ->expects($this->once()) + ->expects($this->exactly(2)) ->method('isEnabled') - ->with('FEATURE_FLAG_UNIQUE_NUMERIC_QTI_IDENTIFIER') - ->willReturn(false); + ->withConsecutive( + ['FEATURE_FLAG_TRANSLATION_ENABLED'], + ['FEATURE_FLAG_UNIQUE_NUMERIC_QTI_IDENTIFIER'] + ) + ->willReturnOnConsecutiveCalls(true, false); + + $originalResourceUriProperty = $this->createPropertyMock(); + $uniqueIdProperty = $this->createPropertyMock(); + + $this->ontology + ->expects($this->exactly(2)) + ->method('getProperty') + ->withConsecutive( + [TaoOntology::PROPERTY_TRANSLATION_ORIGINAL_RESOURCE_URI], + [TaoOntology::PROPERTY_UNIQUE_IDENTIFIER] + ) + ->willReturnOnConsecutiveCalls( + $originalResourceUriProperty, + $uniqueIdProperty + ); + + $this->resource + ->expects($this->once()) + ->method('getOnePropertyValue') + ->with($originalResourceUriProperty) + ->willReturn('originalResourceUri'); + + $originalResource = $this->createResourceMock(); + + $this->ontology + ->expects($this->once()) + ->method('getResource') + ->with('originalResourceUri') + ->willReturn($originalResource); + + $identifier = $this->createMock(core_kernel_classes_Literal::class); + $identifier->literal = 'identifier'; + + $originalResource + ->expects($this->once()) + ->method('getOnePropertyValue') + ->with($uniqueIdProperty) + ->willReturn($identifier); $this->resource ->expects($this->never()) - ->method($this->anything()); + ->method('editPropertyValues'); + + $this->resource + ->expects($this->once()) + ->method('getRootId') + ->willReturn('resourceType'); $this->qtiIdentifierSetter - ->expects($this->never()) - ->method('set'); + ->expects($this->once()) + ->method('set') + ->with([ + AbstractQtiIdentifierSetter::OPTION_RESOURCE => $this->resource, + AbstractQtiIdentifierSetter::OPTION_IDENTIFIER => 'identifier', + ]); $this->sut->__invoke($this->resource); } @@ -84,13 +134,10 @@ public function testUniqueIdFeatureDisabled(): void public function testTranslationFeatureDisabled(): void { $this->featureFlagChecker - ->expects($this->exactly(2)) + ->expects($this->once()) ->method('isEnabled') - ->withConsecutive( - ['FEATURE_FLAG_UNIQUE_NUMERIC_QTI_IDENTIFIER'], - ['FEATURE_FLAG_TRANSLATION_ENABLED'] - ) - ->willReturnOnConsecutiveCalls(true, false); + ->with('FEATURE_FLAG_TRANSLATION_ENABLED') + ->willReturn(false); $this->resource ->expects($this->never()) @@ -106,13 +153,10 @@ public function testTranslationFeatureDisabled(): void public function testNoOriginalResourceUri(): void { $this->featureFlagChecker - ->expects($this->exactly(2)) + ->expects($this->once()) ->method('isEnabled') - ->withConsecutive( - ['FEATURE_FLAG_UNIQUE_NUMERIC_QTI_IDENTIFIER'], - ['FEATURE_FLAG_TRANSLATION_ENABLED'] - ) - ->willReturnOnConsecutiveCalls(true, true); + ->with('FEATURE_FLAG_TRANSLATION_ENABLED') + ->willReturn(true); $originalResourceUriProperty = $this->createPropertyMock(); @@ -142,13 +186,10 @@ public function testNoOriginalResourceUri(): void public function testNoUniqueId(): void { $this->featureFlagChecker - ->expects($this->exactly(2)) + ->expects($this->once()) ->method('isEnabled') - ->withConsecutive( - ['FEATURE_FLAG_UNIQUE_NUMERIC_QTI_IDENTIFIER'], - ['FEATURE_FLAG_TRANSLATION_ENABLED'] - ) - ->willReturnOnConsecutiveCalls(true, true); + ->with('FEATURE_FLAG_TRANSLATION_ENABLED') + ->willReturn(true); $originalResourceUriProperty = $this->createPropertyMock(); $uniqueIdProperty = $this->createPropertyMock(); @@ -199,13 +240,10 @@ public function testNoUniqueId(): void public function testNoQtiIdentifierSetterForSpecifiedType(): void { $this->featureFlagChecker - ->expects($this->exactly(2)) + ->expects($this->once()) ->method('isEnabled') - ->withConsecutive( - ['FEATURE_FLAG_UNIQUE_NUMERIC_QTI_IDENTIFIER'], - ['FEATURE_FLAG_TRANSLATION_ENABLED'] - ) - ->willReturnOnConsecutiveCalls(true, true); + ->with('FEATURE_FLAG_TRANSLATION_ENABLED') + ->willReturn(true); $originalResourceUriProperty = $this->createPropertyMock(); $uniqueIdProperty = $this->createPropertyMock(); @@ -246,7 +284,7 @@ public function testNoQtiIdentifierSetterForSpecifiedType(): void ->willReturn($identifier); $this->resource - ->expects($this->once()) + ->expects($this->never()) ->method('editPropertyValues') ->with($uniqueIdProperty, 'identifier'); @@ -271,8 +309,8 @@ public function testSuccess(): void ->expects($this->exactly(2)) ->method('isEnabled') ->withConsecutive( - ['FEATURE_FLAG_UNIQUE_NUMERIC_QTI_IDENTIFIER'], - ['FEATURE_FLAG_TRANSLATION_ENABLED'] + ['FEATURE_FLAG_TRANSLATION_ENABLED'], + ['FEATURE_FLAG_UNIQUE_NUMERIC_QTI_IDENTIFIER'] ) ->willReturnOnConsecutiveCalls(true, true); From 7f031565ebc568a6f10af75cb9fb4db5dff5829c Mon Sep 17 00:00:00 2001 From: Andrei Shapiro Date: Fri, 14 Feb 2025 14:41:34 +0300 Subject: [PATCH 2/2] chore: generate unique ID property every time even if FF disabled --- .../Service/TranslationUniqueIdSetter.php | 10 +-- .../Service/TranslationUniqueIdSetterTest.php | 78 +------------------ 2 files changed, 7 insertions(+), 81 deletions(-) diff --git a/models/classes/Translation/Service/TranslationUniqueIdSetter.php b/models/classes/Translation/Service/TranslationUniqueIdSetter.php index b228c2ef72..10ead1e900 100644 --- a/models/classes/Translation/Service/TranslationUniqueIdSetter.php +++ b/models/classes/Translation/Service/TranslationUniqueIdSetter.php @@ -71,12 +71,10 @@ public function __invoke(core_kernel_classes_Resource $resource): void AbstractQtiIdentifierSetter::OPTION_IDENTIFIER => $uniqueIdentifier, ]); - if ($this->featureFlagChecker->isEnabled('FEATURE_FLAG_UNIQUE_NUMERIC_QTI_IDENTIFIER')) { - $resource->editPropertyValues( - $this->getProperty(TaoOntology::PROPERTY_UNIQUE_IDENTIFIER), - $uniqueIdentifier - ); - } + $resource->editPropertyValues( + $this->getProperty(TaoOntology::PROPERTY_UNIQUE_IDENTIFIER), + $uniqueIdentifier + ); } private function getOriginalResource(core_kernel_classes_Resource $resource): core_kernel_classes_Resource diff --git a/test/unit/models/classes/Translation/Service/TranslationUniqueIdSetterTest.php b/test/unit/models/classes/Translation/Service/TranslationUniqueIdSetterTest.php index 9954f02b9d..226b5733bb 100644 --- a/test/unit/models/classes/Translation/Service/TranslationUniqueIdSetterTest.php +++ b/test/unit/models/classes/Translation/Service/TranslationUniqueIdSetterTest.php @@ -62,75 +62,6 @@ protected function setUp(): void $this->sut->addQtiIdentifierSetter($this->qtiIdentifierSetter, 'resourceType'); } - public function testUniqueIdFeatureDisabled(): void - { - $this->featureFlagChecker - ->expects($this->exactly(2)) - ->method('isEnabled') - ->withConsecutive( - ['FEATURE_FLAG_TRANSLATION_ENABLED'], - ['FEATURE_FLAG_UNIQUE_NUMERIC_QTI_IDENTIFIER'] - ) - ->willReturnOnConsecutiveCalls(true, false); - - $originalResourceUriProperty = $this->createPropertyMock(); - $uniqueIdProperty = $this->createPropertyMock(); - - $this->ontology - ->expects($this->exactly(2)) - ->method('getProperty') - ->withConsecutive( - [TaoOntology::PROPERTY_TRANSLATION_ORIGINAL_RESOURCE_URI], - [TaoOntology::PROPERTY_UNIQUE_IDENTIFIER] - ) - ->willReturnOnConsecutiveCalls( - $originalResourceUriProperty, - $uniqueIdProperty - ); - - $this->resource - ->expects($this->once()) - ->method('getOnePropertyValue') - ->with($originalResourceUriProperty) - ->willReturn('originalResourceUri'); - - $originalResource = $this->createResourceMock(); - - $this->ontology - ->expects($this->once()) - ->method('getResource') - ->with('originalResourceUri') - ->willReturn($originalResource); - - $identifier = $this->createMock(core_kernel_classes_Literal::class); - $identifier->literal = 'identifier'; - - $originalResource - ->expects($this->once()) - ->method('getOnePropertyValue') - ->with($uniqueIdProperty) - ->willReturn($identifier); - - $this->resource - ->expects($this->never()) - ->method('editPropertyValues'); - - $this->resource - ->expects($this->once()) - ->method('getRootId') - ->willReturn('resourceType'); - - $this->qtiIdentifierSetter - ->expects($this->once()) - ->method('set') - ->with([ - AbstractQtiIdentifierSetter::OPTION_RESOURCE => $this->resource, - AbstractQtiIdentifierSetter::OPTION_IDENTIFIER => 'identifier', - ]); - - $this->sut->__invoke($this->resource); - } - public function testTranslationFeatureDisabled(): void { $this->featureFlagChecker @@ -306,13 +237,10 @@ public function testNoQtiIdentifierSetterForSpecifiedType(): void public function testSuccess(): void { $this->featureFlagChecker - ->expects($this->exactly(2)) + ->expects($this->once()) ->method('isEnabled') - ->withConsecutive( - ['FEATURE_FLAG_TRANSLATION_ENABLED'], - ['FEATURE_FLAG_UNIQUE_NUMERIC_QTI_IDENTIFIER'] - ) - ->willReturnOnConsecutiveCalls(true, true); + ->with('FEATURE_FLAG_TRANSLATION_ENABLED') + ->willReturn(true); $originalResourceUriProperty = $this->createPropertyMock(); $uniqueIdProperty = $this->createPropertyMock();