pkp/pkp-lib#1660 Customizable Reviewer Recommendations

[touhidurabir]

for #1660

[asmecher]

This handler doesn't appear to check that the recommendation being edited/deleted exists in the current context! The manage of one journal can edit and delete recommendations that belong to another.

[touhidurabir]

Isn't that handled by ContextAccessPolicy defined in authorize so that only those have access to context can access that api end point ? And we are restricting the roles to admin, manager and editor .

Or perhaps I am misunderstanding something ?

[asmecher]

Yes, but you're accepting any recommendation ID as user-supplied data -- and that recommendation might belong to another journal that the manager or editor doesn't manage.

[asmecher]

I think it should be possible to eliminate the value column and just rely on a database ID. It would reduce the use of magic numbers, and mean that the options that ship with OJS are treated just like user-created options. To do this, review_assignments.recommendation would also need to be converted to a recommendation_id foreign key to the reviewer_recommendations table.

[touhidurabir]

@asmecher This is something I have been pondering for some time . What you have mentioned is one way to do it that I also thought about. But then we have some complex upgrade process as default values 1-6 is not available anymore when adding to table as primary id will be sequential . So if have 2 contexts , context-1 will have 1-6 and context-2 will have 7-12, so we have to update every review assignments for context-2 . Now for a large installation with many context and many review assignment, upgrade is even more time consuming

1. first need to create recommendations for context
2. map to value 1-6 with recommendations id
3. update all review assignments for that context .

Also need a way to mark the default ones so that on local change, we can add new translations for the default ones (that is not difficult)

The current implementation does not require all these complex process and allow minimum changes to introduce this feature . Thats why I went with this path . But you are right about magic numbers and that can be the major motivation to alter it the other way as you have described . Or maybe I am overthinking here ?

[asmecher]

If we were adding this as a brand new review feature, we would definitely use a foreign key rather than a value field. The only time we need to match a default is when we are adding a new language, and that's a boundary case (which I think is already matching on translation text, not value, right?) -- and that's a rare/boundary case already, so not justifying a different storage method.

The upgrade will be quick, even on large installs:

1. Add a recommendation_id column to review_assignments, default null, with a FK constraint
2. For each journal...
3. Introduce default 6 options to reviewer_recommendations, keeping a PHP-side map from recommendation constant to recommendation_id
4. For each default recommendation option (count=6),
   3. Update any review assignments to use the appropriate ID
5. Drop the recommendation column from review_assignments

That's a fixed number of queries per journal -- no need to e.g. execute anything e.g. per review assignment or per submission.

[asmecher]

Is this change intentional/necessary?

[touhidurabir]

Truthfully I don't actually remember . I will test and confirm .