pkp/pkp-lib#10292 Controlled Vocab DAO to Eloquent Model #10324
Conversation
touhidurabir
force-pushed
the
i10292_main
branch
2 times, most recently
from
5cefa04 to
4845517
Compare
| public function scopeGetLocaleFieldNames(Builder $query): array | ||
| { | ||
| return ['submissionAgency']; | ||
| } |
There was a problem hiding this comment.
Not sure if these methods have any use (here is other similar class to get the locale field name)
touhidurabir
force-pushed
the
i10292_main
branch
from
35ed0cb to
4c2b857
Compare
touhidurabir
force-pushed
the
i10292_main
branch
3 times, most recently
from
d68630a to
3e91b7a
Compare
| fn ($query) => $query->withSymbolic($symbolic)->withAssoc($assocType, $assocId) | ||
| ) | ||
| ->withLocale($locale) | ||
| ->withSetting($symbolic, $value) |
There was a problem hiding this comment.
here previously setting name passed as name but is that correct as for controlled vocabs we have specific settings ?
There was a problem hiding this comment.
Per my comment above -- setting names and controlled vocab symbolic names are different things, and I think this PR mixes them accidentally.
classes/migration/upgrade/v3_5_0/I10292_RemoveControlledVocabEntrySettingType.php
Show resolved
Hide resolved
| return $controlledVocabDao->enumerate($this->getId(), $settingName); | ||
| } | ||
| } | ||
| public function enumerate(?string $settingName = null): array |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
The setting name (the setting_name column in a _settings table) is not the same as the controlled vocab symbolic name, which is the symbolic column in controlled_vocabs. Be careful to keep the two distinct.
There was a problem hiding this comment.
@touhidurabir, I think this comment still applies; you're mixing the symbolic name of the controlled vocabulary with the setting name, and they're different things.
classes/user/interest/Repository.php
Outdated
| 'controlledVocabEntryId' => $interestId, | ||
| ])); | ||
|
|
||
| Repo::controlledVocab()->resequence($controlledVocab->id); |
There was a problem hiding this comment.
do we need to resequence the vocabs for user interest, don't see any benefit or usage of it ?
| $assocId = (DB::table("publications") | ||
| ->select("publication_id as id") | ||
| ->orderBy("publication_id", "desc") | ||
| ->first() | ||
| ->id ?? 0) + 100; |
There was a problem hiding this comment.
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 .
| $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(); |
There was a problem hiding this comment.
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
force-pushed
the
i10292_main
branch
from
da83366 to
e1964a3
Compare
touhidurabir
force-pushed
the
i10292_main
branch
from
160d196 to
bed3f45
Compare
touhidurabir
force-pushed
the
i10292_main
branch
from
02334cd to
9414859
Compare
for #10292