PER-9935 Support V2 requests to /record/copy #151
Conversation
There was a problem hiding this comment.
In investigating the existing copy endpoint, I realized that while this is certainly a useful effort, it isn't actually pressing, the capability that I thought we needed a V2 version for actually exists already. So I think we should still do this, but you're welcome to prioritize other tickets over it.
| const validation = Joi.object() | ||
| .keys({ | ||
| ...fieldsFromUserAuthentication, | ||
| RecordVO: { |
There was a problem hiding this comment.
The concept of a VO isn't used in stela
There was a problem hiding this comment.
Ah, so we don't want to keep backwards compatibility then? I was trying to keep the request format the same.
There was a problem hiding this comment.
Yeah, this does not need to be backwards compatible
| archiveNbr: Joi.string().optional().allow(null), | ||
| folder_linkId: Joi.number().integer().optional().allow(null), | ||
| parentFolder_linkId: Joi.number().integer().optional().allow(null), | ||
| parentFolderId: Joi.number().integer().optional().allow(null), | ||
| folderId: Joi.number().integer().optional().allow(null), | ||
| uploadFileName: Joi.string().optional().allow(null), |
There was a problem hiding this comment.
I don't think we need any of these fields
| @@ -64,3 +66,21 @@ recordController.patch( | |||
| } | |||
| } | |||
| ); | |||
|
|
|||
| recordController.post( | |||
| "/copy", | |||
There was a problem hiding this comment.
I think the path here should be "/{recordId}/copy"
| .keys({ | ||
| ...fieldsFromUserAuthentication, | ||
| RecordVO: { | ||
| recordId: Joi.number().integer().required(), |
There was a problem hiding this comment.
recordId should probably be a URL parameter for this endpoint
| folderId: Joi.number().integer().optional().allow(null), | ||
| uploadFileName: Joi.string().optional().allow(null), | ||
| }, | ||
| FolderDestVO: { folder_linkId: Joi.number().integer().required() }, |
There was a problem hiding this comment.
I'd use folderId rather than folder_linkId. Ideally callers of the API never need know what a folder_link is.
| uploadFileName: Joi.string().optional().allow(null), | ||
| }, | ||
| FolderDestVO: { folder_linkId: Joi.number().integer().required() }, | ||
| archiveId: Joi.number().integer().optional().allow(null), |
There was a problem hiding this comment.
I don't think we need this field?
| // Do we need need to check for read access? | ||
|
|
||
| // Do we need to check for create permission in target folder? |
There was a problem hiding this comment.
Yes. The relevant places to check are the account_archive entries binding the calling account to the archive(s) where the record and destination folder live, plus any access records associated with the record and/or folder
There was a problem hiding this comment.
Also we should check for publish permissions on the record if it's being copied to a public folder
There was a problem hiding this comment.
Also checking for write permissions on the destination should include the check that it's not a connector folder.
| RecordVO: { | ||
| recordId: Joi.number().integer().required(), | ||
| archiveNbr: Joi.string().optional().allow(null), | ||
| folder_linkId: Joi.number().integer().optional().allow(null), | ||
| parentFolder_linkId: Joi.number().integer().optional().allow(null), | ||
| parentFolderId: Joi.number().integer().optional().allow(null), | ||
| folderId: Joi.number().integer().optional().allow(null), | ||
| uploadFileName: Joi.string().optional().allow(null), | ||
| }, | ||
| FolderDestVO: { folder_linkId: Joi.number().integer().required() }, | ||
| archiveId: Joi.number().integer().optional().allow(null), |
There was a problem hiding this comment.
Also note that IDs are always strings in stela. Numeric types should only be used if we expect to do arithmetic or numeric comparison with a value
| export const copyRecord = async ( | ||
| recordData: CopyRecordRequest | ||
| ): Promise<FolderRow> => { | ||
| // TODO check space over - How to check if there is enough space? |
There was a problem hiding this comment.
The account_space table keeps the running total of the space available to an account. The account to check is the calling account, unless the archive where the destination folder lives has payerAccountId set, in which case that's the account to check.
iulianvsp
force-pushed
the
PER-9255-clear-date-on-backend
branch
2 times, most recently
from
e631148
to
c827e36
Compare
No description provided.