Conversation
…n I need to validate the original ticket
| if (published && (doc.level > 0)) { | ||
| const { lastTargetId, lastPosition } = await self.apos.page | ||
| .inferLastTargetIdAndPosition(doc); | ||
| .inferLastTargetIdAndPosition(doc, { publishedTargetsOnly: true }); |
There was a problem hiding this comment.
Page tree editing occurs in draft, but we replay the movement afterwards in the published locale. When doing so we need to make sure we only look at published targets.
| if (doc.level > 0) { | ||
| const { lastTargetId, lastPosition } = await self.apos.page | ||
| .inferLastTargetIdAndPosition(doc); | ||
| .inferLastTargetIdAndPosition(doc, { publishedTargetsOnly: true }); |
There was a problem hiding this comment.
Similarly, insertPublishedOf should only look at published targets.
| // Insert a page. `targetId` must be an existing page id, `_archive` or | ||
| // `_home`, and `position` may be `before`, `inside` or `after`. | ||
| // Alternatively `position` may be a zero-based offset for the new child | ||
| // `_home`, and `position` may be `before`, `after`, `firstChild` or `lastChild`. |
There was a problem hiding this comment.
Unrelated correction to the documentation.
| if (publishedTargetsOnly) { | ||
| const parent = await self.apos.doc.db.findOne({ _id: parentId }); | ||
| if (!parent?.lastPublishedAt) { | ||
| throw self.apos.error('notfound'); |
There was a problem hiding this comment.
We have a strict policy against publishing children of unpublished parents and the UI enforces that etc., so all we need to do is sanity check here. It works out for doc template library because doc templates are children of the home page, and parked pages are always published.
| lastPublishedAt: 1 | ||
| }).toArray(); | ||
| const peers = publishedTargetsOnly | ||
| ? allPeers.filter(peer => (peer._id === doc._id) || peer.lastPublishedAt) |
There was a problem hiding this comment.
We're working with drafts, but consider only those that have a published equivalent (and the page in question itself, that information is used to determine relative ranks).
| position = 'after'; | ||
| } | ||
| if (publishedTargetsOnly) { | ||
| targetId = targetId.replace(':draft', ':published'); |
There was a problem hiding this comment.
Return a published target id.
There was a problem hiding this comment.
Clearer, the inferIdLocaleAndMode also does that, it also mutates req which is hard to debug.
I prefer to do it here, not sure we can get rig of inferIdLocaleAndMode in getTarget.`
|
|
||
| // Publish and assert. | ||
| // Obviously, should not throw an error. | ||
| // Publish them in reverse order, then verify the published ranks |
There was a problem hiding this comment.
I updated these tests a lot for correctness and maintainability / readability.
| path: `${home.aposDocId}/${level1Page2.aposDocId}`, | ||
| level: 1, | ||
| rank: 4, | ||
| rank: 3, |
There was a problem hiding this comment.
I went through this and determined that these new relative ranks are correct. It's just that the new code leaves fewer gaps between ranks.
| }); | ||
| }); | ||
|
|
||
| // We do not support autopublished page types per se, especially |
There was a problem hiding this comment.
Here is my version of your new test, plus a comment for posterity to explain why we have it.
| }); | ||
| }); | ||
|
|
||
| function getIndexes(all, mode, pages) { |
There was a problem hiding this comment.
Utility to reduce boilerplate and return an object with named properties for more readable tests.
ValJed
left a comment
ValJed
left a comment
There was a problem hiding this comment.
Seem a legit workaround if we do not face issues with this diff between draft and published trees. If we publish a draft page, will it move pages around too?
Just style feedback and a concern about parentId that can be an _id or an aposDocId.
| if (publishedTargetsOnly) { | ||
| const parent = await self.apos.doc.db.findOne({ _id: parentId }); | ||
| if (!parent?.lastPublishedAt) { | ||
| throw self.apos.error('notfound'); |
| ? `${parentAposDocId}:${doc.aposLocale}` | ||
| : parentAposDocId; | ||
| if (publishedTargetsOnly) { | ||
| const parent = await self.apos.doc.db.findOne({ _id: parentId }); |
There was a problem hiding this comment.
Looks like parentId can also be an aposDocId if doc has no aposLocale, it might happen only on a non localized site? I still don't think this request would be valid.
Also, are we sure it will request the draft version of the parent? It's ok if we're sure doc is always draft.
There was a problem hiding this comment.
aposDocId and _id are equal for a non localized page so this would not fail in that situation.
| }).toArray(); | ||
| const peers = publishedTargetsOnly | ||
| ? allPeers.filter(peer => (peer._id === doc._id) || peer.lastPublishedAt) | ||
| : allPeers; |
There was a problem hiding this comment.
Don't think you need more data in the projection, same for the filter filter here:
const peerCriteria = {
path: self.matchDescendants(parentPath),
level: doc.level,
...doc.aposLocale && { aposLocale: doc.aposLocale },
...publishedTargetsOnly && { lastPublishedAt: { $ne: null } }
};
const peers = await self.apos.doc.db
.find(peerCriteria)
.sort({ rank: 1 })
.project({ _id: 1 })
.toArray();| position = 'after'; | ||
| } | ||
| if (publishedTargetsOnly) { | ||
| targetId = targetId.replace(':draft', ':published'); |
There was a problem hiding this comment.
Clearer, the inferIdLocaleAndMode also does that, it also mutates req which is hard to debug.
I prefer to do it here, not sure we can get rig of inferIdLocaleAndMode in getTarget.`
2 participants
No description provided.