Port StageAssignmentDAO and StageAssignment to Eloquent Model

[defstat]

Describe the change Right now the StageAssignment entity is a DataObject and it follows the DAO pattern.

We will port StageAssignment entity and its respective DAO class, StageAssignmentDAO to Laravel's Eloquent Model.

What application will be affected? OJS, OMP or OPS main branch PKP supported/maintained plugins main branch

Additional information Please add any screenshots, logs or other information we can use to investigate this bug report.

---

PRs

OJS: pkp/ojs#4166 OMP: pkp/omp#1510 OPS: pkp/ops#626 PKP-LIB: #9675

Plugins Issues and PRs

1. QuickSubmitPlugin - PR: pkp/quickSubmit#91

[defstat]

@Vitaliy-1 this is a PR that removes StageAssignmentDAO and StageAssignment from the codebase in favor of a new StageAssignment Eloquent Model. If you have some time, could you review it, also in relation to your changes at #9589.

Are there things that we should consider adding or develop differently, in order to incorporate the needs of other works in progress?

[Vitaliy-1]

@defstat, please let me know when this is merged so I can backport the changes to the https://github.com/pkp/pkp-lib/issues/8885

[defstat]

@Vitaliy-1 thanks for the code review. I have resolved most of the comments and added 1-2 new comments for clarification. Because there are one-or-two review changes that may affect the tests, I am running the tests on OJS, and waiting for the clarifications on the review.

[Vitaliy-1]

Thanks, @defstat. It looks good! Regarding with...Id() vs with...Ids() methods, leaving it up to you

[defstat]

@asmecher @Vitaliy-1 I have changed the repository functions to the plural form. I have rebased and pushed, the tests seem to be passing.

Before merging them, I would like for you to see the way function updateWithParams of the StageAssignment model

this is letting the user do something like that:

foreach ($stageAssignments as $stageAssignment) {
    $stageAssignment->updateWithParams([
        'canChangeMetadata' => $permitMetadataEdit,
    ]);
}

I had the code like this

$stageAssignments = StageAssignment::withUserGroupId($userGroupId)
                    ->update(['canChangeMetadata' => $permitMetadataEdit]);

which I liked far better, but unfortunately it did not work because of the camel-case syntax of the canChangeMetadata. I should go like

$stageAssignments = StageAssignment::withUserGroupId($userGroupId)
                    ->update(['can_change_metadata' => $permitMetadataEdit]);

giving the actual column name, but by doing that I would have to add DB column name snake-case parameters in the business logic.

What's your opinion on this?

[Vitaliy-1]

Shouldn't Laravel convert the camel case into snake case automatically?

[defstat]

Shouldn't Laravel convert the camel case into snake case automatically?

I thought it should but it seems that in the case of the ->update function it keeps using the DB naming. At least I have not found a way to do it, other than the one I wrote above.

[Vitaliy-1]

Maybe it has something to do with the Illuminate\Database\Eloquent\Concerns\HasAttributes trait?

[defstat]

@Vitaliy-1 If you are referring to Accessors and Mutators I am already using them, but it seems that in the case of ->update the code does not go through that functions. Which makes sense, because those functions work with the values mostly and not the generated query from a function like update.

But if you have something else in mind, I can investigate that further.

I changed the above mentioned function, though, to

public function scopeUpdateWithParams($query, array $attributes)
    {
        $convertedValues = collect($attributes)->mapWithKeys(function ($value, $key) {
            return [Str::snake($key) => $value];
        })->toArray();

        return $query->update($convertedValues);
    }

In that case we can use

StageAssignment::withUserGroupId($userGroupId)
                    ->updateWithParams(['canChangeMetadata' => $permitMetadataEdit]);

which I find much more "eloquent".

So perhaps we could extend the eloquent Model class and use something like a PKPModel where we could perform such non-laravel standard conversions?

Also by using a function like updateWithParams we could explicitly declare to what attribute the DB column name corresponds to, like

$attributeMatching = [
   'canChangeMetadata' => 'can_change_metadata'
] 

and use that in the updateWithParams, or a mix between camelCase to snake_case conversion if the attribute is not declared in $attributeMatching and the explicit conversion if there is declared.

[Vitaliy-1]

@defstat, it seems to work if applied update directly to the model, e.g.:

 $assignment = StageAssignment::withUserGroupId($userGroupId)->first();
 $assignment->update(['canChangeMetadata' => $permitMetadataEdit]);

I'd say that when applying update through the QueryBuilder, it bypasses Model's toArray() method, which performs permutation. Not sure whether the model should use HasAttribute trait or it's optional.

[defstat]

@Vitaliy-1 thanks. I changed the code to update the $stageAssignments one by one using your proposal.

I updated the PRs and the tests seem to be passing.

[defstat]

@Vitaliy-1 @asmecher everything merged on pkp-lib, ojs, omp, ops.

Leaving it open for plugins

[asmecher]

@defstat, is there still work required for this one?

[defstat]

@asmecher I have not yet gone through all plugins for making the necessary changes regarding this.