feature: add support of 'list' type #300

Businesspunk · 2021-10-22T15:22:52Z

TR-1351

Have added support of 'list' type and fix incorrect response in case of 'list' types

How to test (Solar)

Install composer.zip
Publish the test
Complete the test and see the similar response in a result (in var/results)
Run unit-tests

quintanilhar · 2021-10-22T15:24:59Z

src/qtism/common/datatypes/QtiList.php

+ *
+ * Copyright (c) 2013-2020 (original work) Open Assessment Technologies SA (under the project TAO-PRODUCT);
+ *
+ * @author Jérôme Bogaerts <jerome@taotesting.com>


I'm pretty sure he is not the author of this class ;)

Should I write myself?

quintanilhar · 2021-10-22T15:25:21Z

src/qtism/common/datatypes/QtiList.php

+ * 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) 2013-2020 (original work) Open Assessment Technologies SA (under the project TAO-PRODUCT);


Suggested change

* Copyright (c) 2013-2020 (original work) Open Assessment Technologies SA (under the project TAO-PRODUCT);

* Copyright (c) 2021 (original work) Open Assessment Technologies SA (under the project TAO-PRODUCT);

quintanilhar · 2021-10-22T15:26:42Z

src/qtism/common/datatypes/QtiList.php

+     *
+     * @param string $type
+     */
+    private function setBaseType(string $type)


I think both set methods can be removed as they don't provide any extra logic before assigning the value to the attribute. Wdyt?

Yes, I've removed it

test/qtismtest/common/datatypes/ListTest.php

quintanilhar · 2021-10-22T15:32:46Z

src/qtism/runtime/pci/json/Unmarshaller.php

@@ -201,14 +202,32 @@ public function unmarshall($json)

                if (isset($v['base']) || (array_key_exists('base', $v) && $v['base'] === null)) {
                    $unit = ['base' => $v['base']];
+                    $unmarshallItem = $this->unmarshallUnit($unit);
+                } elseif (isset($v['list']) || (array_key_exists('list', $v) && $v['list'] === null)) {


I'm not sure the right side of the condition (array_key_exists('list', $v) && $v['list'] === null) makes sense for this context. I think the else condition is enough in this case.

You mean, we can leave only check on isset there?

$arr = []; $arr['key'] = null; isset($arr['key']) //false

but at the right condition will be true in this keys

but, I was in a hurry, @quintanilhar, you were right here, in this context the first condition is enough

yes @Businesspunk, that's the idea.

quintanilhar · 2021-10-22T15:34:20Z

src/qtism/runtime/pci/json/Unmarshaller.php

+                    $listNewItems = [];
+
+                    $baseType = key($list);
+                    $listItems = $list[$baseType];


Consider removing this variable and using $list[$baseType] in the foreach to avoid creating another variable to hold the result "$listNewItems"

You mean that I have to use only $listItems variable, without creation $listNewItems ?

You can use, you're declaring $listItems just to use in the foreach and then you have another variable $listNewItems, the "new" part sounds a bit weird. So that my proposal is to get rid of the variable $listNewItems.

quintanilhar · 2021-10-22T15:39:33Z

test/qtismtest/common/datatypes/ListTest.php

+        $this::assertTrue($list4->equals($list5));
+    }
+
+    public function testToString()


Suggested change

public function testToString()

public function testConvertsToString()

quintanilhar · 2021-10-22T15:40:57Z

test/qtismtest/common/datatypes/ListTest.php

+
+class ListTest extends QtiSmTestCase
+{
+    public function testEquality()


Consider creating 2 tests for the different outputs:

testReturnsTrueWhenListIsEqualTo
testReturnsFalseWhenListIsDifferent

src/qtism/runtime/pci/json/Unmarshaller.php

poyuki

julien-sebire

I think you'll need the counterpart in src/qtism/pci/json/Marshaller.php

julien-sebire · 2021-10-25T10:54:25Z

src/qtism/common/datatypes/QtiList.php

+ * @license GPLv2
+ */
+
+namespace qtism\common\datatypes;


I understand why you do it this way, but it makes it look like QtiList is a base type. Well, maybe we can do it this way for the sake of simplification, but at least, mention that it's a convenience and kept in qtism/common/datatype for the sake of simplicity.

src/qtism/runtime/pci/json/Unmarshaller.php

quintanilhar

Please, extract the existing implementation to a new method and use it in the record list parsing.

quintanilhar · 2021-10-26T15:49:25Z

test/qtismtest/runtime/pci/json/JsonUnmarshallerTest.php

@@ -375,4 +375,29 @@ public function unmarshallInvalidProvider()
            ['{ "record" : [ { "namez" } ] '],
        ];
    }
+
+    public function testUnmarshallListType()


Suggested change

public function testUnmarshallListType()

public function testUnmarshallListWithinRecord()

poyuki

quintanilhar · 2021-10-27T10:16:46Z

src/qtism/runtime/pci/json/Unmarshaller.php

+     *
+     * @param array $parsedJson
+     * @return MultipleContainer
+     * @throws FileManagerException


I think this exception is not thrown here. Can you confirm, please?

IF type in 'list' will be file it can be

unmarshallFile will be called, which can throw this exception

quintanilhar

Businesspunk requested review from emgolubev, quintanilhar and poyuki October 22, 2021 15:22

quintanilhar reviewed Oct 22, 2021

View reviewed changes

poyuki reviewed Oct 22, 2021

View reviewed changes

test/qtismtest/common/datatypes/ListTest.php Outdated Show resolved Hide resolved

quintanilhar reviewed Oct 22, 2021

View reviewed changes

poyuki reviewed Oct 22, 2021

View reviewed changes

src/qtism/runtime/pci/json/Unmarshaller.php Show resolved Hide resolved

poyuki reviewed Oct 22, 2021

View reviewed changes

src/qtism/runtime/pci/json/Unmarshaller.php Outdated Show resolved Hide resolved

Businesspunk force-pushed the feature/TR-1351/add-supporting-of-list-type branch 2 times, most recently from f53cbc4 to 57d3aad Compare October 22, 2021 16:25

quintanilhar requested a review from julien-sebire October 25, 2021 07:48

Businesspunk force-pushed the feature/TR-1351/add-supporting-of-list-type branch from 57d3aad to 9b9debc Compare October 25, 2021 08:22

poyuki approved these changes Oct 25, 2021

View reviewed changes

julien-sebire requested changes Oct 25, 2021

View reviewed changes

Businesspunk force-pushed the feature/TR-1351/add-supporting-of-list-type branch 4 times, most recently from 423ab79 to 0c67545 Compare October 26, 2021 15:24

Businesspunk requested review from poyuki, julien-sebire and quintanilhar October 26, 2021 15:26

quintanilhar reviewed Oct 26, 2021

View reviewed changes

src/qtism/runtime/pci/json/Unmarshaller.php Show resolved Hide resolved

quintanilhar suggested changes Oct 26, 2021

View reviewed changes

quintanilhar reviewed Oct 26, 2021

View reviewed changes

poyuki approved these changes Oct 26, 2021

View reviewed changes

Businesspunk force-pushed the feature/TR-1351/add-supporting-of-list-type branch 3 times, most recently from bf9f207 to 64f4e91 Compare October 27, 2021 10:01

quintanilhar reviewed Oct 27, 2021

View reviewed changes

Businesspunk force-pushed the feature/TR-1351/add-supporting-of-list-type branch from 64f4e91 to 947ccb5 Compare October 27, 2021 10:25

feature: add support of 'list' type in record

947ccb5

quintanilhar approved these changes Oct 27, 2021

View reviewed changes

Businesspunk merged commit e2b4132 into develop Oct 27, 2021

Businesspunk deleted the feature/TR-1351/add-supporting-of-list-type branch October 27, 2021 12:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: add support of 'list' type #300

feature: add support of 'list' type #300

Businesspunk commented Oct 22, 2021 •

edited

Loading

quintanilhar Oct 22, 2021 •

edited

Loading

Businesspunk Oct 22, 2021

quintanilhar Oct 22, 2021

quintanilhar Oct 22, 2021

Businesspunk Oct 22, 2021

quintanilhar Oct 22, 2021 •

edited

Loading

Businesspunk Oct 22, 2021

poyuki Oct 22, 2021

poyuki Oct 22, 2021

quintanilhar Oct 25, 2021

quintanilhar Oct 22, 2021 •

edited

Loading

Businesspunk Oct 22, 2021

quintanilhar Oct 25, 2021

quintanilhar Oct 22, 2021

quintanilhar Oct 22, 2021

poyuki left a comment

julien-sebire left a comment

julien-sebire Oct 25, 2021

quintanilhar left a comment

quintanilhar Oct 26, 2021

poyuki left a comment

quintanilhar Oct 27, 2021

Businesspunk Oct 27, 2021

Businesspunk Oct 27, 2021

quintanilhar left a comment

	* Copyright (c) 2013-2020 (original work) Open Assessment Technologies SA (under the project TAO-PRODUCT);
	* Copyright (c) 2021 (original work) Open Assessment Technologies SA (under the project TAO-PRODUCT);

	public function testToString()
	public function testConvertsToString()

	public function testUnmarshallListType()
	public function testUnmarshallListWithinRecord()

feature: add support of 'list' type #300

feature: add support of 'list' type #300

Conversation

Businesspunk commented Oct 22, 2021 • edited Loading

How to test (Solar)

quintanilhar Oct 22, 2021 • 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

quintanilhar Oct 22, 2021 • 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

quintanilhar Oct 22, 2021 • 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

poyuki left a comment

Choose a reason for hiding this comment

julien-sebire left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

quintanilhar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

poyuki left a 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

quintanilhar left a comment

Choose a reason for hiding this comment

Businesspunk commented Oct 22, 2021 •

edited

Loading

quintanilhar Oct 22, 2021 •

edited

Loading

quintanilhar Oct 22, 2021 •

edited

Loading

quintanilhar Oct 22, 2021 •

edited

Loading