pkp/pkp-lib#10292 Controlled Vocab DAO to Eloquent Model #10324

touhidurabir · 2024-08-16T11:57:05Z

touhidurabir · 2024-08-20T13:45:25Z

classes/submission/SubmissionAgencyVocab.php

+     * @param int $assocType DO NOT USE: For <3.1 to 3.x migration pkp/pkp-lib#6213
+     * 
+     */
+    public function scopeGetAgencies(


I feel like using the scope methods for these in these extended model classes does not make sense . It does not utilise the scope's methods capabilities and here we had to pass the $builder which has no use . Perhaps a simple static method is good enough .

same for all similar methods .

Just looking at the DO NOT USE note -- is this code that got left over after the 3.0.x to 3.1.x upgrade got removed? (Also more below.)

touhidurabir · 2024-08-20T13:46:29Z

classes/submission/SubmissionAgencyVocab.php

+    public function scopeGetLocaleFieldNames(Builder $query): array
+    {
+        return ['submissionAgency'];
+    }


Not sure if these methods have any use (here is other similar class to get the locale field name)

classes/controlledVocab/ControlledVocab.php

classes/controlledVocab/Repository.php

classes/user/UserInterest.php

asmecher

Thanks, I've added some early review comments!

classes/controlledVocab/ControlledVocab.php

classes/controlledVocab/Repository.php

classes/publication/DAO.php

asmecher · 2024-08-20T21:20:29Z

classes/submission/SubmissionAgencyVocab.php

+     * @param int $assocType DO NOT USE: For <3.1 to 3.x migration pkp/pkp-lib#6213
+     * 
+     */
+    public function scopeGetAgencies(


Just looking at the DO NOT USE note -- is this code that got left over after the 3.0.x to 3.1.x upgrade got removed? (Also more below.)

asmecher · 2024-08-20T21:21:56Z

classes/submission/SubmissionAgencyVocab.php

+use PKP\core\PKPApplication;
+
+
+class SubmissionAgencyVocab extends ControlledVocab


Do you think we could get rid of these classes entirely? If all we're doing is providing a convenience getAgencies function, rather than withSymbolic(SOME_AGENCIES_CONSTANT, ...), then I don't think it's worth the extra classes.

All of there classes have been removed . By removing these extra classes and few related others , our code also reduced significantly .

classes/user/UserInterest.php

touhidurabir · 2024-10-08T11:59:59Z

classes/metadata/MetadataProperty.php

+                                        fn ($query) => $query->withSymbolic($symbolic)->withAssoc($assocType, $assocId)
+                                    )
+                                    ->withLocale($locale)
+                                    ->withSetting($symbolic, $value)


here previously setting name passed as name but is that correct as for controlled vocabs we have specific settings ?

touhidurabir · 2024-10-08T12:02:23Z

classes/migration/upgrade/v3_5_0/I10292_RemoveControlledVocabEntrySettingType.php

+    public function up(): void
+    {
+        Schema::table('controlled_vocab_entry_settings', function (Blueprint $table) {
+            $table->dropColumn('setting_type');
+        });
+    }
+
+    /**
+     * Reverse the migration.
+     */
+    public function down(): void
+    {
+        Schema::table('controlled_vocab_entry_settings', function (Blueprint $table) {
+            $table->string('setting_type', 6);
+        });
+    }


dropping the column setting_type seems reasonable as it seems to have no use and our newer eloquent based implementation for setting tables does not have support for it either .

touhidurabir · 2024-10-08T12:03:53Z

classes/controlledVocab/ControlledVocab.php

-        return $controlledVocabDao->enumerate($this->getId(), $settingName);
-    }
-}
+    public function enumerate(?string $settingName = null): array


Here $settingName previously have default value to name but that seems not right as for controlled vocab, we have a specific sets to settings only . Or I am missing something ?

touhidurabir · 2024-10-08T12:06:39Z

classes/user/interest/Repository.php

+                    'controlledVocabEntryId' => $interestId,
+                ]));
+
+            Repo::controlledVocab()->resequence($controlledVocab->id);


do we need to resequence the vocabs for user interest, don't see any benefit or usage of it ?

touhidurabir · 2024-10-08T12:11:07Z

tests/classes/form/validation/FormValidatorControlledVocabTest.php

+        $assocId = (DB::table("publications")
+            ->select("publication_id as id")
+            ->orderBy("publication_id", "desc")
+            ->first()
+            ->id ?? 0) + 100;


rather than setting a fixed value like 333, it's better to build one based on what we have in DB and add a const value (like here 100) to just to be safe .

touhidurabir · 2024-10-08T12:13:57Z

tests/classes/form/validation/FormValidatorControlledVocabTest.php

+        $testControlledVocab = Repo::controlledVocab()->build(
+            ControlledVocab::CONTROLLED_VOCAB_SUBMISSION_KEYWORD,
+            Application::ASSOC_TYPE_CITATION,
+            $assocId
+        );

-        // Mock the ControlledVocabDAO
-        $mockControlledVocabDao = $this->getMockBuilder(ControlledVocabDAO::class)
-            ->onlyMethods(['getBySymbolic'])
-            ->getMock();
+        $controlledVocabEntryId1 = ControlledVocabEntry::create([
+            'controlledVocabId' => $testControlledVocab->id,
+            ControlledVocab::CONTROLLED_VOCAB_SUBMISSION_KEYWORD => [
+                'en' => 'testEntry',
+            ],
+        ])->id;

-        // Set up the mock getBySymbolic() method
-        $mockControlledVocabDao->expects($this->any())
-            ->method('getBySymbolic')
-            ->with('testVocab', Application::ASSOC_TYPE_CITATION, 333)
-            ->willReturn($mockControlledVocab);
-
-        DAORegistry::registerDAO('ControlledVocabDAO', $mockControlledVocabDao);
+        $controlledVocabEntryId2 = ControlledVocabEntry::create([
+            'controlledVocabId' => $testControlledVocab->id,
+            ControlledVocab::CONTROLLED_VOCAB_SUBMISSION_KEYWORD => [
+                'en' => 'testEntry',
+            ],
+        ])->id;

        // Instantiate validator
-        $validator = new \PKP\form\validation\FormValidatorControlledVocab($form, 'testData', FormValidator::FORM_VALIDATOR_REQUIRED_VALUE, 'some.message.key', 'testVocab', Application::ASSOC_TYPE_CITATION, 333);
+        $validator = new FormValidatorControlledVocab(
+            $form,
+            'testData',
+            FormValidator::FORM_VALIDATOR_REQUIRED_VALUE,
+            'some.message.key',
+            ControlledVocab::CONTROLLED_VOCAB_SUBMISSION_KEYWORD,
+            Application::ASSOC_TYPE_CITATION,
+            $assocId
+        );

-        $form->setData('testData', '1');
+        $form->setData('testData', $controlledVocabEntryId1);
        self::assertTrue($validator->isValid());

-        $form->setData('testData', '2');
+        $form->setData('testData', $controlledVocabEntryId2);
        self::assertTrue($validator->isValid());

-        $form->setData('testData', '3');
+        $form->setData('testData', 3);
        self::assertFalse($validator->isValid());
+
+        // Delete the test vocab along with entries
+        ControlledVocab::find($testControlledVocab->id)->delete();


With implementation of setting table based eloquent, we can not add any arbitrary setting data so only options seems to create data and delete at the end of test . Another option can be is to backup/restore specific tables or full DB but that make the test time high. For this and other similar tests t below .

touhidurabir · 2024-10-20T10:52:56Z

classes/core/SettingsBuilder.php

+        // check if new settings value or multilingual value added and store those to DB
+        $modelWithGivenSettings = static::find($this->model->getKey(), $settingValues->keys()->toArray());
+        $rows = [];
+        $settingValues->each(function (mixed $settingValue, string $settingName) use ($modelWithGivenSettings, &$rows) {
+            $settingName = Str::camel($settingName);
+
+            if (!$modelWithGivenSettings->{$settingName} || is_array($modelWithGivenSettings->{$settingName})) {
+                if ($this->isMultilingual($settingName)) {
+                    $existingLocales = array_keys($modelWithGivenSettings->{$settingName} ?? []);
+
+                    foreach ($settingValue as $locale => $localizedValue) {
+                        if (in_array($locale, $existingLocales)) {
+                            continue;
+                        }
+
+                        $rows[] = [
+                            $this->model->getKeyName() => $this->model->getKey(),
+                            'locale' => $locale,
+                            'setting_name' => $settingName,
+                            'setting_value' => $localizedValue,
+                        ];
+                    }
+                } else {
+                    $rows[] = [
+                        $this->model->getKeyName() => $this->model->getKey(),
+                        'locale' => '',
+                        'setting_name' => $settingName,
+                        'setting_value' => $settingValue,
+                    ];
+                }
+            }
+        });
+
+        if (count($rows) > 0) {
+            DB::table($this->model->getSettingsTable())->insert($rows);
+        }


@Vitaliy-1 this particular change is seems required as when updating a model instance with new settings/multilingual column, it does not add the data in the settings table . for example , a announcement model data may not have descriptionShort or may have descriptionShort for only en . But if we try to update it as following

$announcement->update([ "descriptionShort" => [ "en" => "<p>Test Announcement 2025 nothing</p>" "fr_CA" => "<p>something new</p>" ], ]);

it will not add a new row in the settings table for fr_CA which is expected . I think this will cause issue when updating a multilingual or normal setting field as it will never get inserted . Please take a look of above implementation .

Good spot! It seems that my idea to update settings in 1 query doesn't work if the value wasn't set. Considering the example query:

UPDATE announcement_settings SET setting_value = CASE WHEN setting_name='title' AND locale='en' THEN 'English Title' WHEN setting_name='title' AND locale='fr_CA' THEN 'French TItle' ELSE setting_value END WHERE announcement_id = 16;

I really don't now SQL well enough to say if we can modify it to create a new row if it doesn't exist.
On the other hand, we can just run a series of conventional sql queries to update existing rows or insert new ones. This is a quick example: https://github.com/pkp/pkp-lib/pull/10542/files
Let me know if you want me to resolve this bug

@Vitaliy-1 I am not sure also how to update and insert in a single query and the query may become too complex to work in future too . However I think we need to consider 3 cases here

Updating the existing ones in table in passed through update request .

Adding new ones that do not exists in table passed through update request

deleting what is not passed in the update request .

for for example , if an announcement model instance have 4 settings row for locales en, fr_CA, fr_FR, pt_BR, for title prop and we make a request like

$announcement->update([ "title" => [ "en" => "Title in EN" "fr_CA" => "Title in fr_CA", "fr_FR" => "Title in fr_FR", "el" => "Title in el", ], ]);

So in the above example , we updating en, fr_CA, fr_FR , adding new el and no details about pt_BR which need to be removed . I have added an implementation at https://github.com/pkp/pkp-lib/pull/10324/files#diff-4dcc09fe73d537128c17c4bbb260ad4ba24ccc0ecb8cdc7d0b47ebc7986e2cedR79-R129 but that run 3 queries(update, insert and delete) .

Maybe we can optimize in 2 queries by deleting all for that given props and inserting again. What you think ?

In general, on the object level, all looks OK. All three conditions are met. So in theory we should have all needed info to update data once per row. Looking into it

Aha, we have PKPSchemaService::addMissingMultilingualValues() for API calls that ensures consistency. And we don't have the same logic for Models.

touhidurabir · 2024-10-20T10:55:24Z

classes/core/SettingsBuilder.php

+
+        // TODO : Investigate how to handle the camel to snake case issue. related to pkp/pkp-lib#10485
+        $settingColumns = $this->model->getSettings();
+        $columns = collect($columns)
+            ->map(
+                fn (string $column): string => in_array($column, $settingColumns)
+                    ? $column
+                    : Str::snake($column)
+            )
+            ->toArray();
+
+        foreach ($row as $property => $value) {
+            if (!in_array($property, $columns) && isset($row->{$property})) {


@Vitaliy-1 this change seems required as with current implementation , we do not get the desired outcome with specifying the columns as such

$announcement = Announcement::find(31, ["title"]);

Also throws a lot of warnings . Please take a look .

touhidurabir · 2024-10-20T10:57:24Z

classes/core/traits/ModelWithSettings.php

+        } else {
+            if (!empty($this->fillable)) {
+                $this->mergeFillable(array_merge($this->getSettings(), $this->getMultilingualProps()));
+            }


@Vitaliy-1 this changes seems required to populate the proper fillable as in the if condition, we are only considering entities with schema maps but also need to consider for entities with settings with no schema map .

touhidurabir · 2024-10-21T11:39:06Z

classes/core/traits/ModelWithSettings.php

+    protected function isGuardableColumn($key)
+    {
+        // Need the snake like to key to check for main table to compare with column listing
+        $key = Str::snake($key);
+
+        if (! isset(static::$guardableColumns[get_class($this)])) {
+            $columns = $this->getConnection()
+                        ->getSchemaBuilder()
+                        ->getColumnListing($this->getTable());
+
+            if (empty($columns)) {
+                return true;
+            }
+            static::$guardableColumns[get_class($this)] = $columns;
+        }
+
+
+        $settingsWithMultilingual = array_merge($this->getSettings(), $this->getMultilingualProps());
+        $camelKey = Str::camel($key);
+
+        // Check if this column included in setting and multilingula props and not set to guarded
+        if (in_array($camelKey, $settingsWithMultilingual) && !in_array($camelKey, $this->getGuarded())) {
+            return true;
+        }
+
+        return in_array($key, (array)static::$guardableColumns[get_class($this)]);
+    }


@Vitaliy-1 this override of \Illuminate\Database\Eloquent\Concerns\GuardsAttributes::isGuardableColumn seems required when defining any guarded attributes for entities with no schema map as for schema mapped entities, we always generate the fillable .

touhidurabir · 2024-10-22T12:58:44Z

classes/core/casts/MultilingualSettingAttribute.php

+
+        return [$key => array_filter($value)];
+    }
+}


related to #10476 .

touhidurabir · 2024-10-22T12:59:12Z

classes/core/traits/ModelWithSettings.php

+            $this->generateAttributeCast(
+                collect($this->getMultilingualProps())
+                    ->flatMap(
+                        fn (string $attribute): array => [$attribute => MultilingualSettingAttribute::class]
+                    )
+                    ->toArray()
+            );


related to #10476

touhidurabir · 2024-10-22T13:00:13Z

classes/core/traits/ModelWithSettings.php

+
+            $propCast[$propName] = isset($propSchema->multilingual) && $propSchema->multilingual == true
+                ? MultilingualSettingAttribute::class
+                : $propSchema->type;
        }

-        $propCasts = $this->ensureCastsAreStringValues($propCast);
-        $this->casts = array_merge($propCasts, $this->casts);
+        $this->generateAttributeCast($propCast);
+    }
+
+    /**
+     * Generate the final cast from dynamically generated attr casts
+     */
+    protected function generateAttributeCast(array $attrCast): void
+    {
+        $attrCasts = $this->ensureCastsAreStringValues($attrCast);
+        $this->casts = array_merge($attrCasts, $this->casts);


related to #10476

touhidurabir · 2024-10-22T13:01:20Z

classes/core/SettingsBuilder.php

+        // check if new settings value or multilingual value added and store those to DB
+        $modelWithGivenSettings = static::find($this->model->getKey(), $settingValues->keys()->toArray());
+        $rows = [];
+        $settingValues->each(function (mixed $settingValue, string $settingName) use ($modelWithGivenSettings, &$rows) {
+            $settingName = Str::camel($settingName);
+
+            if (!$modelWithGivenSettings->{$settingName} || is_array($modelWithGivenSettings->{$settingName})) {
+
+                if ($this->isMultilingual($settingName)) {
+                    $existingLocales = array_keys($modelWithGivenSettings->{$settingName} ?? []);
+                    $removeableLocales = array_diff($existingLocales, array_keys($settingValue));
+
+                    foreach ($settingValue as $locale => $localizedValue) {
+                        // Will not add setting entry for locale that already exist 
+                        // as that will be handled by update
+                        if (in_array($locale, $existingLocales)) {
+                            continue;
+                        }
+
+                        // Only add those new locale entries that newly added
+                        $rows[] = [
+                            $this->model->getKeyName() => $this->model->getKey(),
+                            'locale' => $locale,
+                            'setting_name' => $settingName,
+                            'setting_value' => $localizedValue,
+                        ];
+                    }
+
+                    // As the model multilingula attributed is gettting updated
+                    // remove any locale entry associated with setting name not present in the update data need to be removed
+                    if (!empty($removeableLocales)) {
+                        DB::table($this->model->getSettingsTable())
+                            ->where($this->model->getKeyName(), $this->model->getKey())
+                            ->where('setting_name', $settingName)
+                            ->whereIn('locale', $removeableLocales)
+                            ->delete();
+                    }
+                } else {
+                    $rows[] = [
+                        $this->model->getKeyName() => $this->model->getKey(),
+                        'locale' => '',
+                        'setting_name' => $settingName,
+                        'setting_value' => $settingValue,
+                    ];
+                }
+            }
+        });
+
+        if (count($rows) > 0) {
+            DB::table($this->model->getSettingsTable())->insert($rows);
+        }


@Vitaliy-1 updated implementation to handle the update with multilingual prop . continuation of #10324 (comment)

PR

…ns only

…on update

touhidurabir mentioned this pull request Aug 16, 2024

Convert controlled vocabulary classes to Eloquent models #10292

Open

touhidurabir force-pushed the i10292_main branch 2 times, most recently from 5cefa04 to 4845517 Compare August 20, 2024 03:27

touhidurabir commented Aug 20, 2024

View reviewed changes

classes/controlledVocab/ControlledVocab.php Show resolved Hide resolved

touhidurabir commented Aug 20, 2024

View reviewed changes

classes/controlledVocab/Repository.php Outdated Show resolved Hide resolved

touhidurabir commented Aug 20, 2024

View reviewed changes

classes/user/UserInterest.php Outdated Show resolved Hide resolved

touhidurabir commented Aug 20, 2024

View reviewed changes

classes/user/UserInterest.php Outdated Show resolved Hide resolved

asmecher requested changes Aug 20, 2024

View reviewed changes

touhidurabir force-pushed the i10292_main branch from 35ed0cb to 4c2b857 Compare August 26, 2024 13:11

touhidurabir force-pushed the i10292_main branch 3 times, most recently from d68630a to 3e91b7a Compare October 8, 2024 03:58

touhidurabir marked this pull request as ready for review October 8, 2024 11:58

touhidurabir commented Oct 8, 2024

View reviewed changes

touhidurabir force-pushed the i10292_main branch from da83366 to e1964a3 Compare October 18, 2024 13:05

touhidurabir commented Oct 20, 2024

View reviewed changes

touhidurabir requested a review from Vitaliy-1 October 20, 2024 11:00

touhidurabir commented Oct 21, 2024

View reviewed changes

touhidurabir force-pushed the i10292_main branch from 160d196 to bed3f45 Compare October 22, 2024 12:53

touhidurabir commented Oct 22, 2024

View reviewed changes

classes/core/casts/MultilingualSettingAttribute.php

return [$key => array_filter($value)];

}

}

Copy link

Member Author

touhidurabir Oct 22, 2024

Choose a reason for hiding this comment

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

related to #10476 .

touhidurabir commented Oct 22, 2024

View reviewed changes

touhidurabir added 20 commits October 23, 2024 12:30

pkp#10292 Controlled Vocab DAO to Eloquent Model

54bf6b4

pkp#10292 WIP: Controlled Vocab DAO to Eloquent Model

e4d5ee3

pkp#10292 typo fixed

ed09b59

pkp#10292 proper model import for native xml

e5b161c

pkp#10292 fixing unit tests

55f43a9

pkp#10292 Refactored user interest to model and repository

5e2c9f3

pkp#10292 removed extended vocab classes

e924fc3

pkp#10292 fixing issue with vocab storing and some doc blocks added

dd5959d

pkp#10292 added TODO to investigate the impact of merged issue pkp#10423

2a0a379

PR

pkp#10292 refactored entry dao to model class

9ad4168

pkp#10292 removed setting_type column

b321612

pkp#10292 fixed vocab setting issue

a827948

pkp#10292 refactored interest manager to repo and removed it

f192d62

pkp#10292 resolved all but one TODO

6642ae9

pkp#10292 fixed issue the settings builder to retrieve specific colum…

c89621a

…ns only

pkp#10292 added ability to add setting and multilingual column value …

ce24986

…on update

pkp#10292 fixed issue when guarded attribute defined

f12139c

pkp#10292 updated implementation of updating with multilingual data

c4644ba

pkp#10292 added multilingual casting as part of pkp#10476

b9d8651

pkp#10292 fixed issue the data building with settings attrs

6c94c9d

touhidurabir force-pushed the i10292_main branch from fb50a00 to 6c94c9d Compare October 23, 2024 06:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pkp/pkp-lib#10292 Controlled Vocab DAO to Eloquent Model #10324

pkp/pkp-lib#10292 Controlled Vocab DAO to Eloquent Model #10324

touhidurabir commented Aug 16, 2024

touhidurabir Aug 20, 2024 •

edited

Loading

asmecher Aug 20, 2024

touhidurabir Aug 20, 2024 •

edited

Loading

asmecher left a comment

asmecher Aug 20, 2024

asmecher Aug 20, 2024

touhidurabir Oct 8, 2024

touhidurabir Oct 8, 2024

touhidurabir Oct 8, 2024

touhidurabir Oct 8, 2024

touhidurabir Oct 8, 2024

touhidurabir Oct 8, 2024

touhidurabir Oct 8, 2024 •

edited

Loading

touhidurabir Oct 20, 2024

Vitaliy-1 Oct 22, 2024

touhidurabir Oct 23, 2024

Vitaliy-1 Oct 23, 2024

Vitaliy-1 Oct 23, 2024

touhidurabir Oct 20, 2024

touhidurabir Oct 20, 2024

touhidurabir Oct 21, 2024

touhidurabir Oct 22, 2024

touhidurabir Oct 22, 2024

touhidurabir Oct 22, 2024

touhidurabir Oct 22, 2024 •

edited

Loading

		use PKP\core\PKPApplication;


		class SubmissionAgencyVocab extends ControlledVocab

pkp/pkp-lib#10292 Controlled Vocab DAO to Eloquent Model #10324

Are you sure you want to change the base?

pkp/pkp-lib#10292 Controlled Vocab DAO to Eloquent Model #10324

Conversation

touhidurabir commented Aug 16, 2024

touhidurabir Aug 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

touhidurabir Aug 20, 2024 • edited Loading

Choose a reason for hiding this comment

asmecher 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

touhidurabir Oct 8, 2024 • 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

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

touhidurabir Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

touhidurabir Aug 20, 2024 •

edited

Loading

touhidurabir Aug 20, 2024 •

edited

Loading

touhidurabir Oct 8, 2024 •

edited

Loading

touhidurabir Oct 22, 2024 •

edited

Loading