pkp/pkp-lib#9456 Add user private notes #9549
Conversation
| * | ||
| * @return int | ||
| */ | ||
| public function getContextId(): int |
There was a problem hiding this comment.
Indentation is a bit garbled here; there should be a git pre-commit hook that applies automatic formatting. Can you check if that's working?
| } | ||
| } | ||
|
|
||
| if (!PKP_STRICT_MODE) { |
There was a problem hiding this comment.
For new code, it's best not to add class_aliases like this unless for some reason it's necessary (I think that's only for plugin classes at the moment). We'll remove these at some point in the future; they're only for backward compatibility.
| @@ -15,6 +15,7 @@ | |||
| namespace PKP\migration\install; | |||
|
|
|||
| use APP\core\Application; | |||
| use APP\facades\Repo; | |||
There was a problem hiding this comment.
I think this is probably leftover and not necessary.
|
|
||
| use PKP\core\DataObject; | ||
|
|
||
| class UserPrivateNote extends DataObject |
There was a problem hiding this comment.
No need to change this unless you're motivated to -- but in general we're moving towards Eloquent models rather than DataObjects. At some point in the distant future we'll deprecate DataObject entirely.
There was a problem hiding this comment.
I won't have the time to look at this before holidays. I will let this class as a DataObject for now. Do you have any example of an Eloquent model in mind?
There was a problem hiding this comment.
So far just a few relationship classes -- see lib/pkp/classes/userGroup/relationships. We'll be adding more soon (including patterns to demo mapping to the API). If these are proven out by the time you return from holiday, then we can look at adopting them here too; otherwise don't worry about it!
There was a problem hiding this comment.
...but this would be my preference rather than moving to Eloquent.
| * | ||
| * This returns the first user private note, as the "user ID/context ID" key should be unique. | ||
| */ | ||
| public function getFirstUserPrivateNote(int $userId, int $contextId): ?UserPrivateNote |
There was a problem hiding this comment.
Since there's a unique index on this (i.e. it's impossible to have more than one private note per journal per user), I'd suggest dropping First in the function name.
| <?php | ||
|
|
||
| /** | ||
| * @file classes/userPrivateNote/DAO.php |
There was a problem hiding this comment.
Rather than a bespoke table for this feature (with all the attendant management that'll require), another option would've been to use NoteDAO with an assoc_type of ASSOC_TYPE_CONTEXT. Notes are currently used for a few things, and the assoc_type/assoc_id pairing is similar to a Laravel "morphs" relationship.
There was a problem hiding this comment.
We had a couple of issues related to notes in 3.3 (ie random blank empty notes, notes which don't belong to the current publication). Also, I'm not familiar to the concept used by the notes for relationships, but if you really want to avoid the creation of a new table, I can take a look at this in january.
There was a problem hiding this comment.
Yes, I think on balance it'll be worth reusing the notes feature. That would be my preference, above e.g. moving to Laravel or sticking with DAOs. We already have >100 tables and all the attendant classes etc. to go with them, and it's a lot of overhead to maintain!
pkp/ojs#4110