Update MG importer #926

rastislav-chynoransky · 2023-11-28T08:08:33Z

https://jira.sng.sk/browse/MG-14

eronisko

I've gone over the code and I'm providing my suggestions. I'd be happy to discuss any of the points with you in more detail.

In summary:

I'd prefer using the standard Laravel trans helper unless a specific custom behaviour is required
I'm opposed to storing translations and/or topic/medium/work type/etc hierarchy in the database.

I'm expaning on the reasons for 2. in the comments below, but in short:

I believe the translations and hierarchies are of static nature
Having them stored in the database will make development more difficult -- all developers will have to have the same set of translations/hierarchies stored locally (because they're static)
I don't think the benefit of not having to edit a static file every now and then outweighs the drawbacks of managing and distributing the files. (I think we have a pretty good deployment flow set up right now & we can get things fixed & released very quickly)

eronisko · 2023-11-30T09:27:49Z

app/Importers/AbstractImporter.php

+                throw $e;
                $import_record->import->status = Import::STATUS_ERROR;
                $import_record->import->save();


Is this right? I don't think the bottom two lines will get executed

eronisko · 2023-11-30T10:09:50Z

app/Importers/MgImporter.php

+        if ($this->isBiennial($record)) {
+            return $this->translator->get('importer.mg.related_work.biennal_brno', locale: $locale);
+        }


I don't think we need to use any custom translators here. Using something like $this->translator (rather than the stock trans helper) implies that there's some kind of special behaviour needed & I will have to look up the implementation of the custom translator if I want to understand what's going on.

On the other hand something like this is immediately obvious to me (or any Laravel developer)

Suggested change

if ($this->isBiennial($record)) {

return $this->translator->get('importer.mg.related_work.biennal_brno', locale: $locale);

}

if ($this->isBiennial($record)) {

return trans('importer.mg.related_work.biennal_brno', $locale);

}

In this case, I'd even argue that this is not a case for lang files. Because the 'encoding' of the titles is a property of the data source → of the importer, I'd be OK with this as well:

Suggested change

if ($this->isBiennial($record)) {

return $this->translator->get('importer.mg.related_work.biennal_brno', locale: $locale);

}

if ($this->isBiennial($record)) {

return Arr::get([

'sk' => 'Bienále Brno',

'cz' => 'Bienále Brno',

'en' => 'Biennal Brno',

], $locale)

}

In the latter case, there's nothing to look up and everything is very explicit.

app/Importers/MgImporter.php

eronisko · 2023-11-30T10:17:55Z

app/Importers/MgImporter.php

@@ -55,44 +56,7 @@ class MgImporter extends AbstractImporter
        'Te' => 'textil',
    ];


I actually really like(d) this -- I'd just translate it to Slovak instead of Czech and then use that as a basis for translations into other locales. (Using EN as the basis would be OK too but for us I think SK is more suitable)

eronisko · 2023-11-30T10:20:30Z

app/Importers/MgImporter.php

+        if ($record['Titul'] !== null) {
+            return in_array($locale, ['sk', 'cs']) ? $record['Titul'] : null;
+        } elseif ($record['Předmět'] !== null) {
+            return in_array($locale, ['sk', 'cs']) ? $record['Předmět'] : null;
+        } else {
+            return $this->translator->get('item.untitled', locale: $locale);
+        }


nit pick

Suggested change

if ($record['Titul'] !== null) {

return in_array($locale, ['sk', 'cs']) ? $record['Titul'] : null;

} elseif ($record['Předmět'] !== null) {

return in_array($locale, ['sk', 'cs']) ? $record['Předmět'] : null;

} else {

return $this->translator->get('item.untitled', locale: $locale);

}

if ($record['Titul'] !== null) {

return in_array($locale, ['sk', 'cs']) ? $record['Titul'] : null;

}

if ($record['Předmět'] !== null) {

return in_array($locale, ['sk', 'cs']) ? $record['Předmět'] : null;

}

return trans('item.untitled', $locale);

tests/WithoutSearchIndexing.php

eronisko · 2023-11-30T10:34:59Z

tests/Importers/MgImporterTest.php

+    public static function mediumProvider()
+    {
+        return [
+            'medium' => [
+                'papír',
+                null,
+                [
+                    'cs' => 'papír',
+                    'sk' => 'papier',
+                    'en' => 'paper',
+                ],
+            ],
+            'unexistingMedium' => [


This is why I'm not a fan of database-based translations. You have to fill the database just in order to test a piece of fairly trivial functionality. It'll also make it difficult to do any development -- we'd have to maintain the same set of translations for each developer.

In my opinion, there is nothing inherently dynamic about these translations. "Paper" will always be "papier"/"papír" for any gallery, in any environment. So I think these belong into a static form, preferably lang files.

@rastislav-chynoransky are these data providers still needed?

I would like to keep those as the behavior might change for tree structures, i.e. providing at least part of the tree.

eronisko · 2023-11-30T10:59:20Z

app/Medium.php

Same as with the translations. I don't think the hierarchy of media/techniques/topics is dynamic by nature and therefore they do not belong in the database but should be statically defined instead.

For example, we could define the hierarchy like this:

[ 'name' => 'plátno' 'children' => [ [ 'name' => 'plátno na lepenke' ], [ 'name' => 'plátno na preglejke' ], ] ]

Or like this

[ [ 'name' => 'plátno' ], [ 'name' => 'plátno na lepenke' 'parent' => 'plátno' ], [ 'name' => 'plátno na preglejke' 'parent' => 'plátno' ], ]

and that'd capture the same information. Note this would only need to exist for one locale, the translations for other locales would be handled in translation files.

Getting the equivalent of getFullName would be straightforward, without even needing to hit the database.

rastislav-chynoransky · 2023-11-30T13:34:17Z

Thanks for the thorough review.

The reason for storing the filter values in the database is to keep track of missing translations so why not to use them to ease the process of translating new ones. We could still have the translations defined in lang files and use them to populate the entities but that seems a bit redundant.

I don't think you actually need them in dev mode; some exemplary could still be defined in seeders.

This reverts commit 1b3772d.

eronisko

I like this a lot! Please check the dataProviders in tests/Importers/MgImporterTest.php, otherwise this is good to go, thank you.

eronisko · 2023-12-07T07:51:10Z

.github/workflows/_test.yml

@@ -29,6 +29,7 @@ jobs:
        uses: shivammathur/setup-php@v2
        with:
          php-version: '8.1'
+          extensions: :psr


What's this for?

I see now, weird... Looks like something broke upstream?
Could you add a comment explaining the extension addition please? It seems like a temporary requirement to me.

Looks like it's been fixed already, so I guess it could be removed?

Suggested change

extensions: :psr

e.g. https://github.com/SlovakNationalGallery/webumenia.sk/actions/runs/7059939917/job/19401362324

rastislav-chynoransky force-pushed the MG-14 branch 4 times, most recently from e205871 to d98570a Compare November 28, 2023 08:55

rastislav-chynoransky requested a review from eronisko November 28, 2023 08:59

rastislav-chynoransky added 5 commits November 29, 2023 18:57

[import] update mg

10f6f93

[import] remove webumenia mg importer

63c1509

[media] use custom class

1b3772d

[import] remove unused name attribute

e82e3b3

[import] mg defaults

3780fed

rastislav-chynoransky force-pushed the MG-14 branch from 8e032ae to 3780fed Compare November 29, 2023 17:57

eronisko requested changes Nov 30, 2023

View reviewed changes

rastislav-chynoransky added 2 commits December 6, 2023 23:38

[import] @eronisko suggestions

bbf97f7

Revert "[media] use custom class"

85b22ec

This reverts commit 1b3772d.

rastislav-chynoransky requested a review from eronisko December 6, 2023 22:54

[media] use default model class

dd9133f

rastislav-chynoransky force-pushed the MG-14 branch from d65227b to dd9133f Compare December 6, 2023 23:17

eronisko approved these changes Dec 7, 2023

View reviewed changes

eronisko mentioned this pull request Dec 7, 2023

[test] disable psr extension #928

Closed

rastislav-chynoransky force-pushed the MG-14 branch from 279ae04 to dd9133f Compare December 7, 2023 11:26

rastislav-chynoransky merged commit a90b452 into develop Dec 7, 2023
1 of 3 checks passed

rastislav-chynoransky deleted the MG-14 branch December 7, 2023 14:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update MG importer #926

Update MG importer #926

rastislav-chynoransky commented Nov 28, 2023

eronisko left a comment

eronisko Nov 30, 2023

eronisko Nov 30, 2023

eronisko Nov 30, 2023 •

edited

Loading

eronisko Nov 30, 2023

eronisko Nov 30, 2023

eronisko Dec 7, 2023

rastislav-chynoransky Dec 7, 2023

eronisko Nov 30, 2023

rastislav-chynoransky commented Nov 30, 2023

eronisko left a comment

eronisko Dec 7, 2023

eronisko Dec 7, 2023

eronisko Dec 7, 2023

-        if ($this->isBiennial($record)) {
-            return $this->translator->get('importer.mg.related_work.biennal_brno', locale: $locale);
-        }
+        if ($this->isBiennial($record)) {
+            return Arr::get([
+              'sk' => 'Bienále Brno',
+              'cz' => 'Bienále Brno',
+              'en' => 'Biennal Brno',
+            ], $locale)
+        }

		@@ -55,44 +56,7 @@ class MgImporter extends AbstractImporter
		'Te' => 'textil',
		];

Update MG importer #926

Update MG importer #926

Conversation

rastislav-chynoransky commented Nov 28, 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

eronisko Nov 30, 2023 • 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

rastislav-chynoransky commented Nov 30, 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

eronisko Nov 30, 2023 •

edited

Loading