feat(content-uploader): add file version in content uploader API#4581
feat(content-uploader): add file version in content uploader API#4581olehrybak wants to merge 4 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (2)
WalkthroughThis PR threads an ChangesModernized uploads feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
olehrybak
force-pushed
the
uploads-manager-versioning-update
branch
from
3aa5abe to
ddf8e84
Compare
| conflictCallback, | ||
| // $FlowFixMe | ||
| overwrite = true, | ||
| enableModernizedUploads = false, |
There was a problem hiding this comment.
we shouldn't use a feature flag in the APIs files. we should write this in a way that the consumers can opt in or out without it being specific to a feature. let's either utilize the existing overwrite option or add fileVersionNumber
There was a problem hiding this comment.
instead, maybe call it fields based on the previous comment
| if (!uploadUrl) { | ||
| uploadUrl = `${this.getBaseUploadUrl()}/files/content`; | ||
|
|
||
| if (this.fileId) { | ||
| uploadUrl = uploadUrl.replace('content', `${this.fileId}/content`); | ||
| } | ||
| } |
There was a problem hiding this comment.
I think it'd be better to use a fields option which allows for expandability of the API without tying it to a specific feature
if (!uploadUrl) {
uploadUrl = `${this.getBaseUploadUrl()}/files/content`;
if (this.fileId) {
uploadUrl = uploadUrl.replace('content', `${this.fileId}/content`);
}
if (this.fields) {
uploadUrl = updateQueryParameters(uploadUrl, {
fields: this.fields.toString(),
});
}
}
There was a problem hiding this comment.
imo, fields prop would suggest that the fields would be appended to each request in the api, which is not true. I wanted to limit it to only new version upload not to modify any other requests.
Maybe we can keep the current logic, but call the flag something like includeVersionNumber instead of enableModernizedUploads?
| conflictCallback, | ||
| // $FlowFixMe | ||
| overwrite = true, | ||
| enableModernizedUploads = false, |
There was a problem hiding this comment.
instead, maybe call it fields based on the previous comment
| const STANDARD_UPLOAD_FIELDS = [ | ||
| FIELD_DESCRIPTION, | ||
| FIELD_SIZE, | ||
| FIELD_PATH_COLLECTION, | ||
| FIELD_CREATED_AT, | ||
| FIELD_MODIFIED_AT, | ||
| FIELD_TRASHED_AT, | ||
| FIELD_PURGED_AT, | ||
| FIELD_CONTENT_CREATED_AT, | ||
| FIELD_CONTENT_MODIFIED_AT, | ||
| FIELD_CREATED_BY, | ||
| FIELD_MODIFIED_BY, | ||
| FIELD_OWNED_BY, | ||
| FIELD_SHARED_LINK, | ||
| FIELD_PARENT, | ||
| FIELD_ITEM_STATUS, | ||
| ]; | ||
|
|
||
| const STANDARD_UPLOAD_FIELDS_WITH_VERSION_NUMBER = [...STANDARD_UPLOAD_FIELDS, FIELD_VERSION_NUMBER]; |
There was a problem hiding this comment.
following the patterns in this file, I think have one array called UPLOAD_FIELDS_TO_FETCH which has the necessary fields necessary for your feature and then use this just in ContentUploader
|
1st pass from my side +1 on Trevor's comments - especially the recommendation to use a generic also - |
4 participants
Summary
Passed a new
fieldsquery parameter to the file new version upload link. Parameter includes a list of all default fields returned by the upload endpoint along with the additionalversion_numberfield (we need to provide the full list according to docs).The change is guarded by the
enableModernizedUploadsflag.Summary by CodeRabbit
New Features
Refactor