feat: Change to be able to sync test #473

yaraslau-kavaliou · 2024-03-18T08:30:50Z

Ticket - https://oat-sa.atlassian.net/browse/REL-1538
Related ticket - https://github.com/oat-sa/extension-tao-deliver-connect/pull/189

github-actions · 2024-03-18T08:31:52Z

Front-end summary Node 18

💯 Total	✅ Passed	⏭️ Skipped	❌ Failed
13	13	0	0

codecov-commenter · 2024-03-18T08:33:55Z

Codecov Report

Attention: Patch coverage is 37.50000% with 45 lines in your changes are missing coverage. Please review.

Project coverage is 16.21%. Comparing base (e5bf1b6) to head (6a06ac0).

Files	Patch %	Lines
model/DataStore/PersistDataService.php	15.62%	27 Missing ⚠️
model/DataStore/DeliveryMetadataListener.php	52.38%	10 Missing ⚠️
model/DataStore/MetaDataDeliverySyncTask.php	57.89%	8 Missing ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             develop     #473      +/-   ##
=============================================
+ Coverage      15.98%   16.21%   +0.23%     
- Complexity       749      756       +7     
=============================================
  Files             75       75              
  Lines           2659     2701      +42     
=============================================
+ Hits             425      438      +13     
- Misses          2234     2263      +29

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

vbyndych · 2024-03-21T15:35:56Z

model/DataStore/DeliveryMetadataListener.php

@@ -38,7 +38,7 @@ class DeliveryMetadataListener extends ConfigurableService

    public const SERVICE_ID = 'taoDeliveryRdf/DeliveryMetadataListener';

-    public const OPTION_MAX_TRIES = 'max_tries';
+    public const FILE_SYSTEM_ID = 'dataStore';


nitpick: Is it required to have this constant public if it is only used here in this class?

vbyndych · 2024-03-21T15:42:49Z

model/DataStore/DeliveryMetadataListener.php

+                MetaDataDeliverySyncTask::INCLUDE_METADATA_PARAM_NAME => true,
+                MetaDataDeliverySyncTask::MAX_TRIES_PARAM_NAME => $this->getOption('max_tries', 10),
+                MetaDataDeliverySyncTask::FILE_SYSTEM_ID_PARAM_NAME => self::FILE_SYSTEM_ID,
+                MetaDataDeliverySyncTask::IS_REMOVE_PARAM_NAME => false,


nitpick: As I understood this param is responsible for marking tests as deleted in the data storage, so would it be better to call isDelete? This way its intent would be much more clear.

vbyndych · 2024-03-21T15:49:21Z

model/DataStore/MetaDataDeliverySyncTask.php

        $report = new Report(Report::TYPE_SUCCESS);

-        if ($params['count'] < $params[DeliveryMetadataListener::OPTION_MAX_TRIES]) {
+        $params['count'] = $params['count'] ?? 0;


nitpick: seems like we have constants for all params except count do you think we should introduce one?

vbyndych · 2024-03-22T16:48:02Z

model/DataStore/MetaDataDeliverySyncTask.php

@@ -44,30 +44,48 @@ class MetaDataDeliverySyncTask extends AbstractAction implements JsonSerializabl
 {
    use OntologyAwareTrait;

+    public const INCLUDE_DELIVERY_METADATA_PARAM_NAME = 'includeMetadata';


chore: does it make sense to rename the class to match it's new purpose as it is now processing not only deliveries but also tests, what do you think?

In my opinion better would be to have separate classes and reuse common logic. What if we would introduce couple of more entities? How that would look like?

Separate task has been created for test processing

augustas · 2024-04-05T08:13:26Z

model/DataStore/MetaDataDeliverySyncTask.php

-        if (!isset($params['deliveryMetaData'], $params['testMetaData'], $params['testUri'], $params['itemMetaData'])) {
-            $compiler = $this->getMetaDataCompiler();
+        $compiler = $this->getMetaDataCompiler();
+        if ($params[self::INCLUDE_DELIVERY_METADATA_PARAM_NAME]) {


For me it is surprising that same parameter can have both delivery or test id, and that test will be treated as delivery. I think this way it could be confusing, better not mix it up. If it is a test triggering this task, then it is compiled 2 times?

yeah, this is a design smell to me too. It looks like it would be better to have separate events/params.

model/DataStore/MetaDataDeliverySyncTask.php

model/DataStore/DeliveryMetadataListener.php

augustas

I think it could be more explicit that its test package being compiled and uploaded

progys · 2024-04-22T14:07:58Z

model/DataStore/PersistDataService.php

@@ -99,25 +125,28 @@ private function persistArchive(string $deliveryId, array $params): void
     * @throws common_exception_Error
     * @throws common_exception_NotFound
     */
-    private function moveExportedZipTest(string $folder, string $deliveryId, array $params): void
+    private function moveExportedZipTest(string $folder, string $deliveryOrTestId, array $params): void


nitpick: I would rename deliveryOrTestId to entityId in this context it does not look to be important to know that it can hold testId or deliveryId.

P.S. seems to be the case in majority of methods, so please check everywhere.

Replaced to resourceId in this case

augustas · 2024-05-07T16:14:53Z

model/DataStore/PrepareDataService.php

+
+    private function getResourceJsonMetadataCompiler(): ResourceMetadataCompilerInterface
+    {
+        return $this->getServiceManager()->getContainer()->get(ResourceJsonMetadataCompiler::SERVICE_ID);


Probably could do $this->getServiceLocator()->get(ResourceJsonMetadataCompiler::SERVICE_ID);

augustas · 2024-05-08T07:38:52Z

model/DataStore/PrepareDataService.php

+                $testUri = $this->getTestUri($deliveryResource);
+            }
+
+            $test = $this->getResource($resourceId);


This method still is a bit confusing to me, since both delivery or test can arrive here as $resourceId. In order for test metadata to be correct in case $resourceId comes from delivery I believe this should be updated:

Suggested change

$test = $this->getResource($resourceId);

$test = $this->getResource($testUri);

Fixed, thanks for the great catch!

github-actions · 2024-05-08T12:52:28Z

Version

Target Version	14.21.0
Last version	14.20.2

There are 0 BREAKING CHANGE, 7 features, 3 fixes

feat: Change to be able to sync test

97c5049

yaraslau-kavaliou force-pushed the feat/REL-1538/test-ds-sync branch from ba283de to 97c5049 Compare March 18, 2024 08:47

feat: Change to be able to sync test

6a06ac0

yaraslau-kavaliou force-pushed the feat/REL-1538/test-ds-sync branch from 97c5049 to 6a06ac0 Compare March 18, 2024 18:39

yaraslau-kavaliou added 2 commits March 19, 2024 10:54

fix: Conflicts

15a2039

refactor: Create filename method

8cc0cee

yaraslau-kavaliou force-pushed the feat/REL-1538/test-ds-sync branch from 6171fae to 8cc0cee Compare March 19, 2024 08:11

yaraslau-kavaliou requested review from augustas, mccar, vbyndych and olegkhuss March 19, 2024 08:31

vbyndych self-assigned this Mar 21, 2024

yaraslau-kavaliou force-pushed the feat/REL-1538/test-ds-sync branch from e5cc8e3 to 654b4c8 Compare March 21, 2024 17:20

feat: Add test and units metadata to qti test sync

6760bf2

yaraslau-kavaliou force-pushed the feat/REL-1538/test-ds-sync branch from 654b4c8 to 6760bf2 Compare March 21, 2024 17:31

vbyndych requested changes Mar 22, 2024

View reviewed changes

yaraslau-kavaliou added 2 commits April 3, 2024 18:05

feat: Add first tenant id

a597ad5

feat: Add tenant id to data store qti package path

e136c2a

augustas requested changes Apr 5, 2024

View reviewed changes

progys reviewed Apr 22, 2024

View reviewed changes

yaraslau-kavaliou added 4 commits April 29, 2024 20:34

feat: Exchange file by dir during existance checking

d62a397

fix: Issue with test deletion

26251d2

refactor: Delivery saving

05080ec

feat: Add DTO

6e405a8

yaraslau-kavaliou force-pushed the feat/REL-1538/test-ds-sync branch 3 times, most recently from f5c3201 to 0a7abf7 Compare May 6, 2024 18:44

refactor: Delivery data store sync

5580df0

yaraslau-kavaliou force-pushed the feat/REL-1538/test-ds-sync branch from 0a7abf7 to 5580df0 Compare May 6, 2024 18:50

yaraslau-kavaliou requested review from progys, augustas and vbyndych May 7, 2024 10:50

augustas reviewed May 8, 2024

View reviewed changes

fix: Wrong test uri

adf211f

augustas approved these changes May 8, 2024

View reviewed changes

augustas merged commit 54708e5 into develop May 14, 2024
5 of 6 checks passed

augustas deleted the feat/REL-1538/test-ds-sync branch May 14, 2024 15:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Change to be able to sync test #473

feat: Change to be able to sync test #473

yaraslau-kavaliou commented Mar 18, 2024 •

edited

Loading

github-actions bot commented Mar 18, 2024 •

edited

Loading

codecov-commenter commented Mar 18, 2024 •

edited

Loading

vbyndych Mar 21, 2024

yaraslau-kavaliou May 5, 2024 •

edited

Loading

vbyndych Mar 21, 2024

yaraslau-kavaliou May 6, 2024

vbyndych Mar 21, 2024

yaraslau-kavaliou May 6, 2024

vbyndych Mar 22, 2024

progys Apr 22, 2024

yaraslau-kavaliou May 6, 2024

augustas Apr 5, 2024

progys Apr 22, 2024

yaraslau-kavaliou May 6, 2024

augustas left a comment

progys Apr 22, 2024 •

edited

Loading

yaraslau-kavaliou May 6, 2024

augustas May 7, 2024

augustas May 8, 2024

yaraslau-kavaliou May 8, 2024

github-actions bot commented May 8, 2024

	$test = $this->getResource($resourceId);
	$test = $this->getResource($testUri);

feat: Change to be able to sync test #473

feat: Change to be able to sync test #473

Conversation

yaraslau-kavaliou commented Mar 18, 2024 • edited Loading

github-actions bot commented Mar 18, 2024 • edited Loading

Front-end summary Node 18

codecov-commenter commented Mar 18, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

yaraslau-kavaliou May 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

augustas left a comment

Choose a reason for hiding this comment

progys Apr 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 8, 2024

Version

yaraslau-kavaliou commented Mar 18, 2024 •

edited

Loading

github-actions bot commented Mar 18, 2024 •

edited

Loading

codecov-commenter commented Mar 18, 2024 •

edited

Loading

yaraslau-kavaliou May 5, 2024 •

edited

Loading

progys Apr 22, 2024 •

edited

Loading