NEW Add support for ManyManyThrough relations #260

NightJar · 2018-05-20T22:01:56Z

Previously relationships defiend as many_many came in a special type of RelationList (ManyManyList) - however now this can be one of two types of RelationList depending on the type of definition, with both being valid many_many relationships (i.e. ManyManyThrough).

This had the unfortunate side effect of seeing the OrderableRows component in (the least) cease functioning correctly.

This also addresses the inability to version many_many releationships (i.e. many_many_extraFields - e.g. ordering), as the join table itself was not versioned. Using a many_many through allows for this to be versioned via the intermediary join object, as with any other DataObject.

Resolves #254 and resolves #252

NightJar · 2018-05-21T02:21:12Z

Left todo: Find a way to translate the related object ID into the intermediary object ID when setting the sort via reorderItems.
Unit tests for ManyManyThrough.

Previously relationships defiend as many_many came in a special type of RelationList - however now this can be one of two types of RelationList depending on the type of definition, with both being valid many_many relationships. This had the unfortunate side effect of seeing the OrderableRows component in (the least) cease functioning correctly. No longer. This also has the fortunate bonus of allowing a many_many relationship to be versioned; where previously while each item in the relationship could be versioned, the relationship itself could not.

NightJar · 2018-06-01T04:35:50Z

OK, it's a bit mess, but it seems to work :)
The mess can be addressed at another time: #261

thats4shaw · 2018-06-04T02:08:36Z

I gave this a test as per #252 @NightJar.

Seems to work pretty well for me. The only hiccup I had was not versioning the joiner model initially. I'm also assuming in this scenario that the SortOrder field should just be defined in the joiner class rather than use $many_many_extraFields.

NightJar · 2018-06-04T20:53:31Z

Hi @thats4shaw, thanks for giving this a go! Yes you're right, it should now be defined on the intermediary model - I've not looked into whether or not that would work with extraFields, but I assume not.

I inserted some errors to be thrown on the case of mis-matched versioning strategies between the intermediary and the related model - but if I'm honest it's mostly because it hurt my brain trying to think through what it would mean if I didn't. I figure if it remains or becomes an issue it could be dealt with separately (in another issue).

robbieaverill

Couple of minor things and questions - this change definitely needs some unit tests too since its changing quite a lot of important logic

robbieaverill · 2018-06-04T21:27:04Z

src/GridFieldOrderableRows.php

+     * Checks to see if the relationship list is for a type of many_many
+     *
+     * @param SS_List $list
+     */


Missing return type

robbieaverill · 2018-06-04T21:29:08Z

src/GridFieldOrderableRows.php

+            $toRelationName = $manipulator->getLocalKey();
+            $sortlist = DataList::create($joinClass)->filter([
+                $toRelationName => $items->column('ID'),
+                $fromRelationName => $items->first()->getJoin()->$fromRelationName,


What happens if ->first() returns null?

Safe due to earlier checks (short circuit returns) - which I'll also tighten up a touch.

robbieaverill · 2018-06-04T21:30:23Z

src/GridFieldOrderableRows.php

+            $class = $this->getManyManyInspector($list)->getJoinClass();
+            $isVersioned = $class::create()->hasExtension(Versioned::class);
+
+            if ($listClassVersioned xor $isVersioned) {


Is this implying that the base class and a MMT that is owns can't both be versioned?

No, the opposite.

"if the list class is versioned or (never and) the data class versioned"

I'm confused

If one is versioned and the other is not, throw an error.
I'll add a comment :)

ScopeyNZ

Super minor feedback not really blockers for approval I guess.

ScopeyNZ · 2018-06-25T01:22:53Z

src/GridFieldOrderableRows.php

+        //       - Through object is versioned: handle as versioned
+        //       - Through object is NOT versioned: THROW an error.
+        $isManyMany = $this->isManyMany($list);
+        if ($isManyMany && $list instanceof ManyManyThroughList) {


if($isManyMany) is completely redundant here.

ScopeyNZ · 2018-06-25T01:26:02Z

src/GridFieldOrderableRows.php

-            $key   = $list->getLocalKey();
-            $foreignKey = $list->getForeignKey();
+        if ($this->isManyMany($list)) {
+            $intropector = $this->getManyManyInspector($list);


$introspector or $inspector.

robbieaverill

Some minor comments. I'll pull this in and give it a test now

robbieaverill · 2018-06-25T01:32:36Z

src/GridFieldOrderableRows.php

+            $intropector = $this->getManyManyInspector($list);
+            $extra = $list instanceof ManyManyList ?
+                $intropector->getExtraFields() :
+                DataObjectSchema::create()->fieldSpecs($intropector->getJoinClass(), DataObjectSchema::DB_ONLY);


Multi line ternary isn't the best for readability, perhaps you could make this an if statement? Failing that, putting the ? and : at the start of lines helps to indicate that it's part of a ternary condition and not just bad indentation

robbieaverill · 2018-06-25T01:33:44Z

src/GridFieldOrderableRows.php

+     * these functions are moved to ManyManyThroughQueryManipulator, but otherwise retain
+     * the same signature.
+     *
+     * @param ManyManyList|ManyManyThroughList


Missing variable name

robbieaverill · 2018-06-25T01:35:31Z

tests/OrderableRowsThroughTest.yml

+  DefinerOne:
+    ID: 1
+  DefinerTwo:
+    ID: 2


Defining IDs in fixtures isn't a very good idea - you'd ideally let the DB driver assign them via auto increments. Maybe just define a simple varchar for the sake of fixturing instead?

Turns out you don't need to define any values, only names is sufficient :)

robbieaverill · 2018-06-25T01:35:59Z

tests/OrderableRowsThroughVersionedTest.php

+    protected static $required_extensions = [
+        ThroughDefiner::class => [ Versioned::class ],
+        ThroughIntermediary::class => [ Versioned::class ],
+        ThroughBelongs::class => [ Versioned::class ],


Can you remove the spacing around the array values?

robbieaverill · 2018-06-25T01:37:09Z

tests/OrderableRowsThroughVersionedTest.php

+        foreach ($parent->Belongings() as $item) {
+            if ($item->stagesDiffer()) {
+                $this->fail('Unexpected diference found on stages');
+            }


Perhaps you could switch this if statement for $this->assertFalse($item->stagesDiffer(), 'No difference found on stages'); (note typo)

robbieaverill · 2018-06-25T01:38:23Z

tests/OrderableRowsThroughVersionedTest.php

+        $this->assertEquals($desiredOrder, $newOrder);
+
+        // pass forward to testReorderedPublish
+        return $parent;


I don't think is happening, and if it were I think I'd suggest it'd be better not to couple unit tests together. I see you've declared testReorderedPublish as depending on this test, but it doesn't actually use its return value - can you remove

Sorry, can't do. It is passed in at line 86, and used on line 90, 98, & 101 (in testReorderedPublish). I agree that normally it's not a good idea to string tests along like this, but in this case I feel it's acceptable in that if the first test fails there's no point in testing the second as it truly is a prerequisite (in order to publish one must be able to save) - I'm not aware of any method to fixture version information.

It could also be argued that this could all be part of one big test if this dependency is the case, but I feel it is cleaner this way.

Added docblock @param for testReorderedPublish to compliment the comment above the return there and make it a bit clearer :)

Ah I see, the return value is passed into testReorderedPublish as an arg. Ok

robbieaverill · 2018-06-25T01:40:47Z

tests/OrderableRowsThroughVersionedTest.php

+        // there should be no difference between stages anymore
+        foreach ($parent->Belongings() as $item) {
+            if ($item->stagesDiffer()) {
+                $this->fail('Unexpected diference found on stages');


Same comment re: assertFalse($item->stagesDiffer(), ...

The previous commit (9fa9ef8) added support for the new SilverStripe 4 feature of Many Many relationships through an intermediary object. After much head scratching and community testing, the solution was proven to work, however had no automated tests to confirm as such. This commit rectifies that by testing both versioned and unversioned DataObjects in a many_many through style relationship. Some minor tidy and comments were also added as per feedback on the functionality code changes.

robbieaverill

LGTM 😃 nice work!

gorriecoe · 2018-07-03T21:19:09Z

When is this likely to be tagged? This fixes a major issue for our team so ideally this week if possible?

robbieaverill · 2018-07-03T21:31:09Z

@gorriecoe here you go: https://github.com/symbiote/silverstripe-gridfieldextensions/releases/tag/3.2.0

NightJar added type/enhancement affects/v4 change/minor complexity/high impact/high labels May 20, 2018

NightJar mentioned this pull request May 20, 2018

FIX Work with ManyManyThroughList #258

Closed

robbieaverill changed the title ~~WIP - NEW Add support for ManyManyThrough relations~~ WIP: NEW Add support for ManyManyThrough relations May 21, 2018

NightJar force-pushed the pulls/3.1/many-many-reordering-fix branch from 851b9b9 to 872b04f Compare May 31, 2018 22:33

NightJar force-pushed the pulls/3.1/many-many-reordering-fix branch from 872b04f to 9fa9ef8 Compare June 1, 2018 04:24

NightJar mentioned this pull request Jun 1, 2018

Dead code and unreferenced variables should be removed #261

Open

NightJar changed the title ~~WIP: NEW Add support for ManyManyThrough relations~~ NEW Add support for ManyManyThrough relations Jun 1, 2018

This was referenced Jun 1, 2018

Orderable Rows not Sorting many_many relationship with extraFields #252

Closed

NEW Update RelatedPages feature for ManyManyThrough silverstripe/cwp#93

Merged

NightJar mentioned this pull request Jun 4, 2018

GridFieldOrderableRows - No support for ManyManyThroughList #254

Closed

robbieaverill suggested changes Jun 4, 2018

View reviewed changes

NightJar changed the title ~~NEW Add support for ManyManyThrough relations~~ WIP: NEW Add support for ManyManyThrough relations Jun 10, 2018

ScopeyNZ reviewed Jun 25, 2018

View reviewed changes

NightJar force-pushed the pulls/3.1/many-many-reordering-fix branch from 1ac7b66 to c98d710 Compare June 25, 2018 01:40

robbieaverill suggested changes Jun 25, 2018

View reviewed changes

NightJar force-pushed the pulls/3.1/many-many-reordering-fix branch 6 times, most recently from 08ed6f6 to 1eaa4c8 Compare June 25, 2018 03:36

NightJar force-pushed the pulls/3.1/many-many-reordering-fix branch from 1eaa4c8 to b6130c4 Compare June 26, 2018 02:41

NightJar changed the title ~~WIP: NEW Add support for ManyManyThrough relations~~ NEW Add support for ManyManyThrough relations Jun 26, 2018

robbieaverill mentioned this pull request Jun 28, 2018

Invalid SQL query when reordering items via GridFieldOrderableRows for the first time #241

Closed

robbieaverill approved these changes Jul 1, 2018

View reviewed changes

robbieaverill merged commit 7fdfe23 into symbiote:master Jul 1, 2018

robbieaverill deleted the pulls/3.1/many-many-reordering-fix branch July 1, 2018 21:59

kinglozzer mentioned this pull request Jan 21, 2019

"ManyManyThrough cannot mismatch Versioning between join class and related class" #282

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NEW Add support for ManyManyThrough relations #260

NEW Add support for ManyManyThrough relations #260

NightJar commented May 20, 2018 •

edited by robbieaverill

Loading

NightJar commented May 21, 2018

NightJar commented Jun 1, 2018

thats4shaw commented Jun 4, 2018

NightJar commented Jun 4, 2018

robbieaverill left a comment

robbieaverill Jun 4, 2018

robbieaverill Jun 4, 2018

NightJar Jun 24, 2018

robbieaverill Jun 4, 2018

NightJar Jun 6, 2018

robbieaverill Jun 6, 2018

NightJar Jun 10, 2018

ScopeyNZ left a comment

ScopeyNZ Jun 25, 2018

ScopeyNZ Jun 25, 2018

NightJar Jun 25, 2018

robbieaverill left a comment

robbieaverill Jun 25, 2018

robbieaverill Jun 25, 2018

robbieaverill Jun 25, 2018

NightJar Jun 25, 2018

robbieaverill Jun 25, 2018

robbieaverill Jun 25, 2018

robbieaverill Jun 25, 2018

NightJar Jun 25, 2018

NightJar Jun 25, 2018

robbieaverill Jun 25, 2018

robbieaverill Jun 25, 2018

robbieaverill left a comment

gorriecoe commented Jul 3, 2018

robbieaverill commented Jul 3, 2018

NEW Add support for ManyManyThrough relations #260

NEW Add support for ManyManyThrough relations #260

Conversation

NightJar commented May 20, 2018 • edited by robbieaverill Loading

NightJar commented May 21, 2018

NightJar commented Jun 1, 2018

thats4shaw commented Jun 4, 2018

NightJar commented Jun 4, 2018

robbieaverill 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

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

ScopeyNZ 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

robbieaverill 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

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

robbieaverill left a comment

Choose a reason for hiding this comment

gorriecoe commented Jul 3, 2018

robbieaverill commented Jul 3, 2018

NightJar commented May 20, 2018 •

edited by robbieaverill

Loading