Artist role (autor) - parse from artist name and store in separately #898

rastislav-chynoransky · 2023-09-17T19:37:14Z

https://jira.sng.sk/browse/WEBUMENIA-1533

eronisko

I'm sure this code works but I'm having a hard time understanding it. Could you please take a look at the suggestions and/or could we have chat about this?

eronisko · 2023-09-21T11:45:48Z

app/Item.php

+            'authority_item',
+            'item_id',
+            'authority_id'
+        )->withPivot(['role', 'automatically_matched']);


Could this work without selecting automatically_matched globally?

I find it more convenient to have it listed globally especially when one boolean attribute creates not much of overhead.

I get the convenience aspect but I'd still suggest not including it globally. The problem with these global includes is that they are very difficult to track down if we ever want to change or remove that column later on. Having to say withPivot() is annoying but it is much more explicit.

This is coming from experience, I've used this feature many times and it always bit me in the end 😄

eronisko · 2023-09-21T12:17:16Z

app/Item.php

+    {
+        $idsToDetach = $this->authorities
+            ->filter(fn(Authority $authority) => $authority->pivot->automatically_matched)
+            ->pluck('id')
+            ->diff($idsWithPivotData->keys());
+
+        $this->authorities()->detach($idsToDetach);
+        $this->authorities()->syncWithoutDetaching(
+            $idsWithPivotData->map(
+                fn($pivotData) => [
+                    'automatically_matched' => true,
+                    ...$pivotData,
+                ]
+            )
+        );
+    }


Just to make sure I understand: this removes all automatically_matched authorities that are not present in $idsWithPivotData keys?

Would this work too?

Suggested change

{

$idsToDetach = $this->authorities

->filter(fn(Authority $authority) => $authority->pivot->automatically_matched)

->pluck('id')

->diff($idsWithPivotData->keys());

$this->authorities()->detach($idsToDetach);

$this->authorities()->syncWithoutDetaching(

$idsWithPivotData->map(

fn($pivotData) => [

'automatically_matched' => true,

...$pivotData,

]

)

);

}

{

$this->authorities()->wherePivot('automatically_matched', true)->sync(

$idsWithPivotData->map(

fn($pivotData) => [

'automatically_matched' => true,

...$pivotData,

]

)

);

}

If so, could it be inlined instead? (I can only see it used once)

This approach won't work because the wherePivot condition will limit the scope of pivot records, effectively leading to inserts of duplicate pivots if there already exist some with automatically_matched = false

And for now this change is made only for GMUHK gallery, however it will get reused in CSV importers (or any new data source).

@eronisko I've included your suggestion discussed via slack 84c5076

eronisko · 2023-09-21T12:29:58Z

tests/Models/ItemTest.php

@@ -184,4 +186,44 @@ protected function createFreeItem()
            'date_latest' => 1, // CE
        ]);
    }
+
+    public function testSyncMatchedAuthorities()


I'm very happy to see a test here but I'm having trouble understanding what it does :(

Could you perhaps work it into a more higher-level test case for GmuhkItemImporter?

Maybe a test like this

public function testParseRoleFromArtistName() { $data = [...]; GmuhkItemImporter::import($data); $this->assertEquals('expected role', Item::first()->authorities[0]->role) }

In fact the test makes multiple assertions and could be split into five scenarios:

automatically matched that are present in newly matched are expected to stay there (with updated pivot data)

manually matched that are present in newly matched are expected to stay there (with updated pivot data)

automatically matched that are missing from newly matched are expected to be deleted

manually matched that are missing from newly matched are expected to stay there

and finally any new matches should be created

Of course I can create a higher-level test for importing role, but the test of these relationship changes should stay on the model-level.

added higher-level test 892ca5b

…authorities

eronisko

I've got a couple more suggestions with the global withPivot being a serious concern. Otherwise, this is good to go, thank you.

eronisko · 2023-09-22T12:05:26Z

app/Item.php

+            'authority_item',
+            'item_id',
+            'authority_id'
+        )->withPivot(['role', 'automatically_matched']);


I get the convenience aspect but I'd still suggest not including it globally. The problem with these global includes is that they are very difficult to track down if we ever want to change or remove that column later on. Having to say withPivot() is annoying but it is much more explicit.

This is coming from experience, I've used this feature many times and it always bit me in the end 😄

eronisko · 2023-09-22T12:06:09Z

tests/Harvest/Importers/GmuhkItemImporterTest.php

+        Authority::factory()->create([
+            'name' => 'Miroslav Podhrázský',
+            'birth_year' => null,
+            'death_year' => null,
+        ]);
+
+        $row = FakeRecordFactory::buildGmuhkItem([
+            'author' => ['Miroslav Podhrázský (fotograf)'],
+        ]);
+        $item = $this->importer->import($row, new Progress());
+        $item->refresh();
+        $this->assertEquals('fotograf', $item->authorities[0]->pivot->role);


Very nice, thank you

eronisko · 2023-09-22T12:18:46Z

app/Item.php

+    public function syncMatchedAuthorities(\Illuminate\Support\Collection $idsWithPivotData)
+    {
+        // Detach automatically-matched authorities that no longer match


Thanks for this, just last suggestion to accept array as well (in order to be more in-line with Laravel's sync method)

Suggested change

public function syncMatchedAuthorities(\Illuminate\Support\Collection $idsWithPivotData)

{

// Detach automatically-matched authorities that no longer match

public function syncMatchedAuthorities(\Illuminate\Support\Collection|array $idsWithPivotData)

{

$idsWithPivotData = collect($idsWithPivotData);

// Detach automatically-matched authorities that no longer match

eronisko · 2023-09-22T12:22:00Z

tests/Models/ItemTest.php

+        $authorities = Authority::factory()
+            ->count(5)
+            ->create();
+        $item = Item::factory()->create();
+        $item->authorities()->sync([
+            // present in matched authorities
+            $authorities[0]->id => ['role' => 'author', 'automatically_matched' => true],
+            $authorities[1]->id => ['role' => 'author', 'automatically_matched' => false],
+            // missing from matched authorities
+            $authorities[2]->id => ['role' => 'author', 'automatically_matched' => true],
+            $authorities[3]->id => ['role' => 'author', 'automatically_matched' => false],
+        ]);
+
+        $item->syncMatchedAuthorities(
+            collect([
+                $authorities[0]->id => ['role' => 'after'],
+                $authorities[1]->id => ['role' => 'after'],
+                $authorities[4]->id => ['role' => 'after'],
+            ])
+        );
+
+        $item->refresh();
+        $this->assertCount(4, $item->authorities);
+        $this->assertEquals(
+            [
+                $authorities[0]->id => ['role' => 'after', 'automatically_matched' => true],
+                $authorities[1]->id => ['role' => 'after', 'automatically_matched' => false],
+                $authorities[3]->id => ['role' => 'author', 'automatically_matched' => false],
+                $authorities[4]->id => ['role' => 'after', 'automatically_matched' => true],
+            ],
+            $item->authorities
+                ->pluck('pivot')
+                ->keyBy('authority_id')
+                ->map->only(['role', 'automatically_matched'])
+                ->toArray()
+        );


Here's a more chatty version. I find it a bit easier to digest but I'm not 100% happy with this either. Feel free to use/adapt it or throw away.

Suggested change

$authorities = Authority::factory()

->count(5)

->create();

$item = Item::factory()->create();

$item->authorities()->sync([

// present in matched authorities

$authorities[0]->id => ['role' => 'author', 'automatically_matched' => true],

$authorities[1]->id => ['role' => 'author', 'automatically_matched' => false],

// missing from matched authorities

$authorities[2]->id => ['role' => 'author', 'automatically_matched' => true],

$authorities[3]->id => ['role' => 'author', 'automatically_matched' => false],

]);

$item->syncMatchedAuthorities(

collect([

$authorities[0]->id => ['role' => 'after'],

$authorities[1]->id => ['role' => 'after'],

$authorities[4]->id => ['role' => 'after'],

])

);

$item->refresh();

$this->assertCount(4, $item->authorities);

$this->assertEquals(

[

$authorities[0]->id => ['role' => 'after', 'automatically_matched' => true],

$authorities[1]->id => ['role' => 'after', 'automatically_matched' => false],

$authorities[3]->id => ['role' => 'author', 'automatically_matched' => false],

$authorities[4]->id => ['role' => 'after', 'automatically_matched' => true],

],

$item->authorities

->pluck('pivot')

->keyBy('authority_id')

->map->only(['role', 'automatically_matched'])

->toArray()

);

$item = Item::factory()->create();

[$authority1, $authority2] = Authority::factory()

->count(2)

->create();

// Updates existing authorities

$item->authorities()->sync([

$authority1->id => ['role' => 'author', 'automatically_matched' => true],

$authority2->id => ['role' => 'author', 'automatically_matched' => false],

]);

$item->syncMatchedAuthorities([

$authority1->id => ['role' => 'new-role-1'],

$authority2->id => ['role' => 'new-role-2'],

]);

$this->assertEqualsCanonicalizing(

collect(['new-role-1', 'new-role-2']),

$item->authorities()->pluck('role')

);

// Deletes auto-matched authorities, keeps manually matched

$item->authorities()->sync([

$authority1->id => ['automatically_matched' => true],

$authority2->id => ['automatically_matched' => false],

]);

$item->syncMatchedAuthorities(collect([]));

$this->assertCount(1, $item->authorities()->get());

$this->assertEquals($authority2->id, $item->authorities()->first()->id);

// Adds new authorities

$item->authorities()->sync([]);

$item->syncMatchedAuthorities([

$authority1->id => ['role' => 'author'],

]);

$this->assertCount(1, $item->authorities()->get());

rastislav-chynoransky force-pushed the WEBUMENIA-1533 branch 2 times, most recently from 5d39ad2 to 077587a Compare September 17, 2023 19:59

rastislav-chynoransky requested a review from eronisko September 17, 2023 20:04

[item] sync matched authorities

534b0de

rastislav-chynoransky force-pushed the WEBUMENIA-1533 branch from 077587a to a3b5a87 Compare September 17, 2023 20:08

eronisko requested changes Sep 21, 2023

View reviewed changes

rastislav-chynoransky added 2 commits September 21, 2023 23:42

[import] gmuhk authority roles

7cfc144

[import] gmuhk item test

892ca5b

rastislav-chynoransky force-pushed the WEBUMENIA-1533 branch from a3b5a87 to 892ca5b Compare September 21, 2023 21:43

[item] keep manually matched flag when syncing automatically matched …

84c5076

…authorities

rastislav-chynoransky requested a review from eronisko September 22, 2023 11:49

eronisko approved these changes Sep 22, 2023

View reviewed changes

rastislav-chynoransky merged commit 2cf05dc into develop Sep 22, 2023
1 check passed

rastislav-chynoransky deleted the WEBUMENIA-1533 branch September 22, 2023 12:27

rastislav-chynoransky mentioned this pull request Sep 28, 2023

Apply suggestions from PR#898 #902

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Artist role (autor) - parse from artist name and store in separately #898

Artist role (autor) - parse from artist name and store in separately #898

rastislav-chynoransky commented Sep 17, 2023

eronisko left a comment

eronisko Sep 21, 2023

rastislav-chynoransky Sep 21, 2023

eronisko Sep 22, 2023

eronisko Sep 21, 2023

rastislav-chynoransky Sep 21, 2023

rastislav-chynoransky Sep 21, 2023

rastislav-chynoransky Sep 22, 2023

eronisko Sep 21, 2023

rastislav-chynoransky Sep 21, 2023

rastislav-chynoransky Sep 21, 2023

eronisko left a comment

eronisko Sep 22, 2023

eronisko Sep 22, 2023

eronisko Sep 22, 2023

eronisko Sep 22, 2023

Artist role (autor) - parse from artist name and store in separately #898

Artist role (autor) - parse from artist name and store in separately #898

Conversation

rastislav-chynoransky commented Sep 17, 2023

eronisko 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

eronisko 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