Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

make changes for import/export module to work properly #4270

Merged
merged 26 commits into from
Oct 12, 2023
Merged
Show file tree
Hide file tree
Changes from 22 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
30ab5da
pass checked document ids to toolbar
ETLaurent Aug 15, 2023
5cbec97
use dismissProgressNotification
ETLaurent Aug 23, 2023
c714926
pass notification options and sent results as an event in notif
ETLaurent Aug 23, 2023
3b06f5e
remove notification options and use req.body.messages to sent results…
ETLaurent Aug 23, 2023
7c94ea6
adds the option originalResponse to the http remote method for the ba…
ValJed Aug 28, 2023
ccb5c18
disable export for users
ETLaurent Aug 29, 2023
eaf5179
importExport
ETLaurent Aug 30, 2023
b376997
adds new color variable
ValJed Sep 18, 2023
fc15e25
allows to pass existing ID to attachment insert method
ValJed Sep 18, 2023
bea905a
when passing attachmentId check if this one exists already
ValJed Sep 20, 2023
f1eb181
adds update method
ValJed Sep 21, 2023
a560591
returns promise in order to wait if needed
ValJed Sep 22, 2023
5346f26
starts notification with percentage of 0 to avoid NaN
ValJed Sep 26, 2023
e9f7a0c
removes an useless return satement
ValJed Sep 26, 2023
66989c4
in attachment update method, make sure to delete the existing retriev…
ValJed Sep 28, 2023
2b62ecf
Merge remote-tracking branch 'origin/main' into pro-4261-export-docs
ETLaurent Oct 2, 2023
6d88220
fixes lint
ValJed Oct 3, 2023
26df2d0
adds changelog
ValJed Oct 3, 2023
b1b72c2
minor changes
ValJed Oct 4, 2023
2c6f00a
compute new docIds and archivedDocIds when updating attachment to ver…
ValJed Oct 6, 2023
734e292
moves import export related options to the module itself
ValJed Oct 6, 2023
90f9fb3
only gets existing related docs from IDs
ValJed Oct 6, 2023
92b8054
Merge remote-tracking branch 'origin/main' into pro-4261-export-docs
ETLaurent Oct 11, 2023
526c519
typos
ETLaurent Oct 12, 2023
8af5ac0
typo
ETLaurent Oct 12, 2023
4ba007d
use docIds and archivedDocIds from existing attachment to get related…
ETLaurent Oct 12, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,13 @@
# Changelog

## Unreleased

### Add

* Allows to insert attachments with a given ID, as well as with `docIds` and `atchiveDocIds` to preserve related docs.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Allows to insert attachments with a given ID, as well as with `docIds` and `atchiveDocIds` to preserve related docs.
* Allows to insert attachments with a given ID, as well as with `docIds` and `archiveDocIds` to preserve related docs.

* Adds an `update` method to the attachment module, that update the mongoDB doc and the associated file.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Adds an `update` method to the attachment module, that update the mongoDB doc and the associated file.
* Adds an `update` method to the attachment module, that updates the mongoDB doc and the associated file.

* Adds an option to the `http` `remote` method to allow receiving the original response from `node-fetch` that is a stream.

## 3.57.0 2023-09-27

### Adds
Expand Down
56 changes: 50 additions & 6 deletions modules/@apostrophecms/attachment/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -386,8 +386,7 @@ module.exports = {
// This method returns `attachment` where `attachment` is an attachment
// object, suitable for passing to the `url` API and for use as the value
// of a `type: 'attachment'` schema field.
async insert(req, file, options) {
options = options || {};
async insert(req, file, options = {}) {
let extension = path.extname(file.name);
if (extension && extension.length) {
extension = extension.substr(1);
Expand All @@ -402,16 +401,21 @@ module.exports = {
extensions: accepted.join(req.t('apostrophe:listJoiner'))
}));
}

if (options.attachmentId && await self.apos.attachment.db.findOne({ _id: options.attachmentId })) {
throw self.apos.error('invalid', 'duplicate');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could hit a race condition and imposes an unnecessary delay, try/catch the insert and check for self.apos.doc.isUniqueError(err) to see if mongodb threw a unique key error or something else.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed I kept the behavior as it is for now, we might create a ticket to update slightly the insert method in order to insert the doc before to copy to file to uploadfs.
At least here if a user use the option attachmentId (I don't see a lot of use case) they are protected from a mess (file created while the document has not been inserted).

}

const info = {
_id: self.apos.util.generateId(),
_id: options.attachmentId ?? self.apos.util.generateId(),
group: group.name,
createdAt: new Date(),
name: self.apos.util.slugify(path.basename(file.name, path.extname(file.name))),
title: self.apos.util.sortify(path.basename(file.name, path.extname(file.name))),
extension: extension,
type: 'attachment',
docIds: [],
archivedDocIds: []
docIds: options.docIds ?? [],
archivedDocIds: options.archivedDocIds ?? []
};
if (!(options.permissions === false)) {
if (!self.apos.permission.can(req, 'upload-attachment')) {
Expand All @@ -432,7 +436,11 @@ module.exports = {
}
if (self.isSized(extension)) {
// For images we correct automatically for common file extension mistakes
const result = await Promise.promisify(self.uploadfs.copyImageIn)(file.path, '/attachments/' + info._id + '-' + info.name, { sizes: self.imageSizes });
const result = await Promise.promisify(self.uploadfs.copyImageIn)(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only indentation.

file.path,
'/attachments/' + info._id + '-' + info.name,
{ sizes: self.imageSizes }
);
info.extension = result.extension;
info.width = result.width;
info.height = result.height;
Expand All @@ -454,6 +462,42 @@ module.exports = {
await self.db.insertOne(info);
return info;
},

async update(req, file, attachment) {
const existing = await self.db.findOne({ _id: attachment._id });
if (!existing) {
throw self.apos.error('notfound');
}

const projection = {
_id: 1,
archived: 1
};

const existingRelatedDocs = await self.apos.doc.db.find({
_id: { $in: [ ...attachment.docIds, ...attachment.archivedDocIds ] }
}, { projection }).toArray();

const { docIds, archivedDocIds } = existingRelatedDocs
.reduce(({ docIds, archivedDocIds }, doc) => {
return {
docIds: [ ...docIds, ...!doc.archived ? [ doc._id ] : [] ],
archivedDocIds: [ ...archivedDocIds, ...doc.archived ? [ doc._id ] : [] ]
};
}, {
docIds: [],
archivedDocIds: []
});

await self.alterAttachment(existing, 'remove');
await self.db.deleteOne({ _id: existing._id });
await self.insert(req, file, {
attachmentId: attachment._id,
docIds: _.uniq([ ...docIds, ...existing.docIds || [] ]),
archivedDocIds: _.uniq([ ...archivedDocIds, ...existing.archivedDocIds || [] ])
});
},

// Given a path to a local svg file, sanitize any XSS attack vectors that
// may be present in the file. The caller is responsible for catching any
// exception thrown and treating that as an invalid file but there is no
Expand Down
4 changes: 4 additions & 0 deletions modules/@apostrophecms/http/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,7 @@ module.exports = {
// `fullResponse` (if true, return an object with `status`, `headers` and `body`
// properties, rather than returning the body directly; the individual `headers` are canonicalized
// to lowercase names. If a header appears multiple times an array is returned for it)
// `originalResponse` (if true, return the response object exactly as it is returned by node-fetch)
//
// If the status code is >= 400 an error is thrown. The error object will be
// similar to a `fullResponse` object, with a `status` property.
Expand Down Expand Up @@ -225,6 +226,9 @@ module.exports = {
options.jar.setCookieSync(cookie, url);
});
}
if (options.originalResponse) {
return res;
}
awaitedBody = true;
body = await getBody();
if (res.status >= 400) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@
:labels="moduleLabels"
:disable="relationshipErrors === 'min'"
:displayed-items="items.length"
:checked="checked"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh, was this just an unrelated bug?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a bug, it has been added to be consistent with the other vue changes.
Basically the modal for exporting documents needed the checked IDs so we pass it to AposDocsManagerToolbar when used.

:checked-count="checked.length"
:module-name="moduleName"
@page-change="updatePage"
Expand Down
12 changes: 10 additions & 2 deletions modules/@apostrophecms/job/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,7 @@ module.exports = {
await self.triggerNotification(req, 'completed', {
count: total,
dismiss: true
});
}, results);
// Dismiss the progress notification. It will delay 4 seconds
// because "completed" notification will dismiss in 5 and we want
// to maintain the feeling of process order for users.
Expand All @@ -261,11 +261,18 @@ module.exports = {
// array
// No messages are required, but they provide helpful information to
// end users.
async triggerNotification(req, stage, options = {}) {
async triggerNotification(req, stage, options = {}, results) {
if (!req.body || !req.body.messages || !req.body.messages[stage]) {
return {};
}

const event = req.body.messages.resultsEventName && results
? {
name: req.body.messages.resultsEventName,
data: { ...results }
}
: null;

ValJed marked this conversation as resolved.
Show resolved Hide resolved
return self.apos.notification.trigger(req, req.body.messages[stage], {
interpolate: {
count: options.count || (req.body._ids && req.body._ids.length),
Expand All @@ -277,6 +284,7 @@ module.exports = {
action: options.action,
ids: options.ids
},
event,
icon: req.body.messages.icon || 'database-export-icon',
type: options.type || 'success',
return: true
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,10 @@ export default {
type: Number,
required: true
},
checked: {
type: Array,
default: () => []
},
checkedCount: {
type: Number,
required: true
Expand Down Expand Up @@ -273,7 +277,7 @@ export default {
modal, ...rest
}) {
await apos.modal.execute(modal, {
count: this.checkedCount,
checked: this.checked,
moduleName: this.moduleName,
...rest
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ export default {
route: `${apos.modules['@apostrophecms/job'].action}/${this.notification.job._id}`,
processed: 0,
total: 1,
percentage: 0,
action: this.notification.job.action
} : null
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@
:labels="moduleLabels"
:displayed-items="items.length"
:is-relationship="!!relationshipField"
:checked="checked"
:checked-count="checked.length"
:batch-operations="moduleOptions.batchOperations"
:module-name="moduleName"
Expand Down
1 change: 1 addition & 0 deletions modules/@apostrophecms/ui/ui/apos/scss/global/_theme.scss
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
--a-success: #00bf9a;
--a-success-fade: #00bf9a30;
--a-warning: #ffce00;
--a-warning-dark: #a75c07;
--a-warning-fade: #ffce0030;
--a-progress-bg: #2c354d;

Expand Down
Loading