Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feature] Overwrite test by URI during QTI import #2491

Merged
merged 10 commits into from
Aug 13, 2024
130 changes: 106 additions & 24 deletions actions/class.RestQtiTests.php
Original file line number Diff line number Diff line change
Expand Up @@ -35,11 +35,21 @@
class taoQtiTest_actions_RestQtiTests extends AbstractRestQti
{
public const PARAM_PACKAGE_NAME = 'qtiPackage';

private const PARAM_TEST_URI = 'testUri';

private const ITEM_CLASS_URI = 'itemClassUri';

/**
* @deprecated Use taoQtiTest_actions_RestQtiTests::OVERWRITE_TEST_URI instead with the URI of the test to be
* replaced
*/
private const OVERWRITE_TEST = 'overwriteTest';

private const SUBCLASS_LABEL = 'subclassLabel';
private const OVERWRITE_TEST_URI = 'overwriteTestUri';
private const PACKAGE_LABEL = 'packageLabel';

/**
* @throws common_exception_NotImplemented
*/
Expand Down Expand Up @@ -99,7 +109,9 @@ public function import()
$this->isItemMustExistEnabled(),
$this->isItemMustBeOverwrittenEnabled(),
$this->isOverwriteTest(),
$this->getItemClassUri()
$this->getItemClassUri(),
$this->getOverwriteTestUri(),
$this->getPackageLabel()
);

if ($report->getType() === common_report_Report::TYPE_SUCCESS) {
Expand All @@ -117,16 +129,27 @@ public function import()
}
return $this->returnSuccess($data);
} else {
throw new \common_exception_RestApi($report->getMessage());
throw new common_exception_RestApi($report->getMessage());
}
} catch (\common_exception_RestApi $e) {
} catch (common_exception_RestApi $e) {
return $this->returnFailure($e);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shpran would it be asking too much if we could make sure that the exceptions caught here have logs with:

  • Exception class name that was triggered
  • Exception message
  • Exception trace

Maybe we already have something on returnFailure implementation?

I am asking, cause generally this logs with only exception message do not help at all as we have no idea where it happened. This is a critical process for the whole ingestion process, so I think it is useful to have this logs

Wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one doesn't relate to my changes (just fixed FQN), but sure I will improve these logs 😎
Thanks for suggestion

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shpran I know this is not part of your changes, but today is a bit of a pain to track errors when we are generating translations. Could you please add logs for that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked returnFailuer implementation and found, that we use Advanced Logger feature there, so message, file, line and trace should be logged:
image

}
}

protected function getItemClassUri(): ?string
{
return $this->getPostParameter(self::ITEM_CLASS_URI);
$itemClassUri = $this->getPostParameter(self::ITEM_CLASS_URI);
$subclassLabel = $this->getSubclassLabel();

if ($subclassLabel) {
foreach ($this->getClass($itemClassUri)->getSubClasses() as $subclass) {
if ($subclass->getLabel() === $subclassLabel) {
$itemClassUri = $subclass->getUri();
}
}
}

return $itemClassUri;
}

/**
Expand Down Expand Up @@ -176,7 +199,9 @@ public function importDeferred()
$this->isItemMustExistEnabled(),
$this->isItemMustBeOverwrittenEnabled(),
$this->isOverwriteTest(),
$this->getItemClassUri()
$this->getItemClassUri(),
$this->getOverwriteTestUri(),
$this->getPackageLabel()
);

$result = [
Expand All @@ -191,7 +216,7 @@ public function importDeferred()
}

return $this->returnSuccess($result);
} catch (\common_exception_RestApi $e) {
} catch (common_exception_RestApi $e) {
return $this->returnFailure($e);
gabrielfs7 marked this conversation as resolved.
Show resolved Hide resolved
}
}
Expand Down Expand Up @@ -254,27 +279,22 @@ protected function addExtraReturnData(EntityInterface $taskLogEntity)
* If parent class parameter is an uri of valid test class, new class will be created under it
* If not parent class parameter is provided, class will be created under root class
* Comment parameter is not mandatory, used to describe new created class
*
* @return \core_kernel_classes_Class
*/
public function createClass()
public function createClass(): void
{
try {
$class = $this->createSubClass(new \core_kernel_classes_Class(TaoOntology::CLASS_URI_TEST));
$class = $this->createSubClass(new core_kernel_classes_Class(TaoOntology::CLASS_URI_TEST));

$result = [
$this->returnSuccess([
'message' => __('Class successfully created.'),
'class-uri' => $class->getUri(),
];

$this->returnSuccess($result);
} catch (\common_exception_ClassAlreadyExists $e) {
$result = [
]);
} catch (common_exception_ClassAlreadyExists $e) {
$this->returnSuccess([
'message' => $e->getMessage(),
'class-uri' => $e->getClass()->getUri(),
];
$this->returnSuccess($result);
} catch (\Exception $e) {
]);
} catch (Exception $e) {
$this->returnFailure($e);
}
}
Expand All @@ -295,20 +315,82 @@ private function getUploadedPackageData()
$mimeType = tao_helpers_File::getMimeType($fileData['tmp_name']);

if (!in_array($mimeType, self::$accepted_types)) {
throw new \common_exception_RestApi(__('Wrong file mime type'));
throw new common_exception_RestApi(__('Wrong file mime type'));
}

return $fileData;
}

/**
* @return string|null
* @throws common_exception_RestApi
*/
private function getSubclassLabel(): ?string
{
$subclassLabel = $this->getPostParameter(self::SUBCLASS_LABEL);

if ($subclassLabel !== null && !is_string($subclassLabel)) {
throw new common_exception_RestApi(
sprintf('%s parameter should be string', self::SUBCLASS_LABEL)
);
}

return $subclassLabel;
}

/**
* @return string|null
* @throws common_exception_RestApi
*/
private function getOverwriteTestUri(): ?string
{
$overwriteTestUri = $this->getPostParameter(self::OVERWRITE_TEST_URI);

if ($overwriteTestUri !== null && !is_string($overwriteTestUri)) {
throw new common_exception_RestApi(
sprintf('%s parameter should be string', self::OVERWRITE_TEST_URI)
);
}

return $overwriteTestUri;
}

/**
* @return string|null
* @throws common_exception_RestApi
*/
private function getPackageLabel(): ?string
{
$packageLabel = $this->getPostParameter(self::PACKAGE_LABEL);

if ($packageLabel !== null && !is_string($packageLabel)) {
throw new common_exception_RestApi(
sprintf('%s parameter should be string', self::PACKAGE_LABEL)
);
}

return $packageLabel;
}

/**
* Get class instance to import test
* @throws \common_exception_RestApi
* @return \core_kernel_classes_Class
*
* @throws common_exception_RestApi
*/
private function getTestClass()
private function getTestClass(): core_kernel_classes_Class
{
return $this->getClassFromRequest(new \core_kernel_classes_Class(TaoOntology::CLASS_URI_TEST));
$testClass = $this->getClassFromRequest(new core_kernel_classes_Class(TaoOntology::CLASS_URI_TEST));
$subclassLabel = $this->getSubclassLabel();

if ($subclassLabel) {
foreach ($testClass->getSubClasses() as $subClass) {
if ($subClass->getLabel() === $subclassLabel) {
$testClass = $subClass;
}
}
}

return $testClass;
}

/**
Expand Down
32 changes: 30 additions & 2 deletions doc/swagger.json
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,21 @@
{
"name": "overwriteTest",
"in": "formData",
"description": "Flag that indicates if already existed tests and all related items are removed in a case they have the same title and class as uploaded tests. Can be used to replace existing test and avoid creating a new one with the same title and class. 'true' value counts as true, 'false' as false",
"description": "Deprecated: use `overwriteTestUri` parameter with the URI of the test to be replaced. Flag that indicates if already existed tests and all related items are removed in a case they have the same title and class as uploaded tests. Can be used to replace existing test and avoid creating a new one with the same title and class. 'true' value counts as true, 'false' as false",
"type": "string",
"required": false
},
{
"name": "overwriteTestUri",
"in": "formData",
"description": "Test URI, that can be used to replace existing test and avoid creating a new one with the same title and class",
"type": "string",
"required": false
},
{
"name": "packageLabel",
"in": "formData",
"description": "Label for items folder and test label, which will be used instead of QTI test title",
"type": "string",
"required": false
},
Expand Down Expand Up @@ -353,7 +367,21 @@
{
"name": "overwriteTest",
"in": "formData",
"description": "Flag that indicates if already existed tests and all related items are removed in a case they have the same title and class as uploaded tests. Can be used to replace existing test and avoid creating a new one with the same title and class. 'true' value counts as true, 'false' as false",
"description": "Deprecated: use `overwriteTestUri` parameter with the URI of the test to be replaced. Flag that indicates if already existed tests and all related items are removed in a case they have the same title and class as uploaded tests. Can be used to replace existing test and avoid creating a new one with the same title and class. 'true' value counts as true, 'false' as false",
"type": "string",
"required": false
},
{
"name": "overwriteTestUri",
"in": "formData",
"description": "Test URI, that can be used to replace existing test and avoid creating a new one with the same title and class",
"type": "string",
"required": false
},
{
"name": "packageLabel",
"in": "formData",
"description": "Label for items folder and test label, which will be used instead of QTI test title",
"type": "string",
"required": false
},
Expand Down
37 changes: 35 additions & 2 deletions models/classes/class.CrudQtiTestsService.php
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
*
*/

use oat\oatbox\log\LoggerAwareTrait;
use oat\tao\model\TaoOntology;

/**
Expand All @@ -32,6 +33,8 @@
*/
class taoQtiTest_models_classes_CrudQtiTestsService extends tao_models_classes_CrudService
{
use LoggerAwareTrait;

/** (non-PHPdoc)
* @see tao_models_classes_CrudSservice::getClassService()
*/
Expand Down Expand Up @@ -67,7 +70,9 @@ public function importQtiTest(
$itemMustExist = false,
$itemMustBeOverwritten = false,
bool $overwriteTest = false,
?string $itemClassUri = null
?string $itemClassUri = null,
?string $overwriteTestUri = null,
?string $packageLabel = null
) {
try {
//The zip extraction is a long process that can exceed the 30s timeout
Expand All @@ -91,11 +96,39 @@ public function importQtiTest(
$importer->enableItemMustBeOverwritten();
}

$report = $importer->importMultipleTests($class, $uploadedFile, $overwriteTest, $itemClassUri);
$report = $importer->importMultipleTests(
$class,
$uploadedFile,
$overwriteTest,
$itemClassUri,
[],
$overwriteTestUri,
$packageLabel
);
helpers_TimeOutHelper::reset();

return $report;
} catch (common_exception_UserReadableException $e) {
$this->logException($e);

return new common_report_Report(common_report_Report::TYPE_ERROR, __($e->getUserMessage()));
gabrielfs7 marked this conversation as resolved.
Show resolved Hide resolved
} catch (Throwable $exception) {
$this->logException($exception);

return new common_report_Report(common_report_Report::TYPE_ERROR, __('QTI test import failed'));
}
}

private function logException(Throwable $exception): void
{
$this->logError(
sprintf(
'QTI test import failed: %s. File: %s, Line: %s. Stack trace: %s',
$exception->getMessage(),
$exception->getFile(),
$exception->getLine(),
$exception->getTraceAsString()
)
);
}
}
Loading
Loading