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

adds method to sort all documents and pages #101

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

ValJed
Copy link
Contributor

@ValJed ValJed commented Jan 29, 2025

PRO-6827

Summary

Fixes page hierarchy not being respected during import.
Sort exported documents as well as their related documents.

What are the specific steps to test this change?

For example:

  1. Run the website and log in as an admin
  2. Open a piece manager modal and select several pieces
  3. Click the "Archive" button on the top left of the manager and confirm that it should proceed
  4. Check that all pieces have been archived properly

What kind of change does this PR introduce?

  • Bug fix
  • New feature
  • Refactor
  • Documentation
  • Build-related changes
  • Other

Make sure the PR fulfills these requirements:

  • It includes a) the existing issue ID being resolved, b) a convincing reason for adding this feature, or c) a clear description of the bug it resolves
  • The changelog is updated
  • Related documentation has been updated
  • Related tests have been updated

@@ -133,7 +170,7 @@ module.exports = self => {
$in: publishedIds
}
})
.relationships(includeRelationships)
.relationships(false)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

unneeded since we get related docs manually

Copy link
Member

Choose a reason for hiding this comment

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

Avoiding the redundant call should be a nice little perf win then.

@@ -121,7 +158,7 @@ module.exports = self => {
$in: draftIds
}
})
.relationships(includeRelationships)
.relationships(false)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

unneeded since we get related docs manually

Copy link
Member

Choose a reason for hiding this comment

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

BC issue: someone may have overridden this method, so please check for 5 arguments and if there are five, accept them that way too, ignoring the redundant parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Riiiight BC, always forget that. Would be so good to have a public API, and internal methods that shouldn't be updated by users.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@ValJed ValJed requested a review from boutell January 29, 2025 10:14
Copy link
Member

@boutell boutell left a comment

Choose a reason for hiding this comment

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

Good strategy, minor issue.

@@ -7,7 +7,7 @@ const { uniqBy, uniqueId } = require('lodash');

const MAX_RECURSION = 10;

module.exports = self => {
module.exports = (self) => {
Copy link
Member

Choose a reason for hiding this comment

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

Any special reason? (I don't object)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

None, for consistency, I thought we used to do this way. But not in this project, reverting.

@@ -33,9 +33,11 @@ module.exports = self => {

const hasRelatedTypes = !!relatedTypes.length;

const docs = (await self.getDocs(req, ids, hasRelatedTypes, manager, reporting))
const docs = (await self.getDocs(req, ids, manager, reporting))
Copy link
Member

Choose a reason for hiding this comment

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

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since we do not get relationships whatever if export related docs has been checked or no, we don't need this param anymore.

@@ -121,7 +158,7 @@ module.exports = self => {
$in: draftIds
}
})
.relationships(includeRelationships)
.relationships(false)
Copy link
Member

Choose a reason for hiding this comment

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

BC issue: someone may have overridden this method, so please check for 5 arguments and if there are five, accept them that way too, ignoring the redundant parameter.

@@ -133,7 +170,7 @@ module.exports = self => {
$in: publishedIds
}
})
.relationships(includeRelationships)
.relationships(false)
Copy link
Member

Choose a reason for hiding this comment

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

Avoiding the redundant call should be a nice little perf win then.

return relatedTypes.some(type => {
const module = self.apos.modules[type];
return self.apos.instanceOf(module, '@apostrophecms/page-type');
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@boutell I can move this function in a utils file if you think it makes no sense to attach it to self (which I would agree with I think).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants