PRO-6418: parked pages & singletons #74

myovchev · 2024-08-19T10:42:04Z

Summary

All singleton documents (parked pages and pieces having option singleton: true) are now marked as "duplicates" in order to get a user confirmation for override. This happen ONLY if the singleton document exists in the target site database. If it doesn't exist, the document is handled as a new generic document (fallback to the previous behavior).

The UI now accepts replaceIds pairs beside the usual dociIds. Those are sent back on confirmation (only for the checked documents). The backend is using them to perform document ID replacement while importing and for updating all document relationships at the end of the import process.

What are the specific steps to test this change?

Export parked pages (such as @apostrophecms/home-page, @apostrophecms/search-page) and a singleton piece(s) (such as @apostrophecms/global). Ensure that those documents have some relationships and attachments (e.g. images)
Import the previous exported in another (blank) site (e.g. change the database after the export). Ensure that all of the imported singletons are keeping the ID's from the target site. Ensure that all reverse relationships (in the related documents) have the proper ID's (the target site documents).

What kind of change does this PR introduce?

(Check at least one)

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

If adding a new feature without an already open issue, it's best to open a feature request issue first and wait for approval before working on it.

Other information:

linear · 2024-08-19T10:42:07Z

PRO-6418 Solution for handling imports of parked pages and singletons that already exist on the target site

myovchev · 2024-08-19T16:01:36Z

package.json

@@ -17,7 +18,7 @@
  "author": "Apostrophe Technologies",
  "license": "UNLICENSED",
  "devDependencies": {
-    "apostrophe": "github:apostrophecms/apostrophe",
+    "apostrophe": "github:apostrophecms/apostrophe#pro-6418-parked-singletons",


FIXME: Revert before merge/after core is merged.

ValJed

Few questions feedback

modules/@apostrophecms/import-export-doc-type/index.js

ValJed · 2024-08-20T15:39:11Z

ui/apos/components/AposDuplicateImportModal.vue

+        aposDocId => this.duplicatedDocs.find(doc => doc.replaceId && doc.aposDocId === aposDocId)
+      )
+        .filter(Boolean)
+        .map(({ aposDocId, replaceId }) => [ aposDocId, replaceId ]);


Maybe an object instead of an array would be clearer, and would avoid things like replaceItem[1] which is not really clear I think?

this follows the backend argument requirements - see await self.apos.doc.changeDocIds(replaceDocIdPairs, { skipReplace: true }); and the relevant code in overrideDuplicates.
For consistency, every "pair" is in that format.

modules/@apostrophecms/import-export-doc-type/index.js

ValJed · 2024-08-20T15:44:43Z

ui/apos/components/AposImportModal.vue

+      width: calc(100% + 60px);
+      height: 100%;
+      content: "";
+      background-color: var(--a-base-9);


Does these changes are about linting?

Auto formatted by stylelint.

ui/apos/components/AposExportModal.vue

lib/methods/import.js

ValJed · 2024-08-21T07:27:33Z

.stylelintrc.json

+      "files": ["*.s{c,a}ss", "**/*.s{c,a}ss"],
+      "customSyntax": "postcss-scss"
+    }
+  ]


This should not be needed, it already exists in stylelint-config-apostrophe. No extra config should be needed to use the same config in each project/

Example error this fix:

λ npm run stylelint > @apostrophecms/[email protected] stylelint > stylelint ui/**/*.{scss,vue} ui/apos/components/AposDuplicateImportModal.vue 366:21 ✖ Unexpected unit "s" for property declaration-property-unit-allowed-list "transition" 366:21 ✖ Expected a minimum of 200 time-min-milliseconds milliseconds 2 problems (2 errors, 0 warnings)

Same when removing it:

λ npm run stylelint > @apostrophecms/[email protected] stylelint > stylelint ui/**/*.{scss,vue} /apostrophe/import-export/ui/apos/components/AposDuplicateImportModal.vue: you should use the "customSyntax" option when linting something other than CSS /apostrophe/import-export/ui/apos/components/AposExportModal.vue: you should use the "customSyntax" option when linting something other than CSS /apostrophe/import-export/ui/apos/components/AposImportModal.vue: you should use the "customSyntax" option when linting something other than CSS ui/apos/components/AposDuplicateImportModal.vue 15:16 ✖ Unknown word CssSyntaxError ui/apos/components/AposExportModal.vue 15:16 ✖ Unknown word CssSyntaxError ui/apos/components/AposImportModal.vue 15:16 ✖ Unknown word CssSyntaxError 3 problems (3 errors, 0 warnings)

Ok this is because you don't use the latest version of our stylelint config.
Please upgrade to 4.1 and remove the added config.

lib/methods/import.js

ValJed · 2024-08-21T08:05:46Z

lib/methods/import.js

+            });
+            continue;
+          }
+        }


[Optional]: Maybe we could have used a Map here to store singletons list id -> replaceId in order to use the same array map above that build the duplicatedDocs and having another loop on replaceId.
Just an idea here not sure it would really improve code readability / maintainability here

I've tried many variants, including trying to extract the whole logic in a function. The real problem is the complex continue logic (extremely sensitive to bugs). I prefer to keep it as it is now until we have really good idea how to do it.
I think the current is clean because its 2 phase logic - 1. either replace or duplicate (no change) set, 1.5. duplicate set only logic (no change), 2. append replace set directly to the result, it's prebuilt.

A better design is to move the whole logic (for a single document) in a separate method. This would "relax" the code depth and open possibilities for improvements. I don't think we are still there/forced to do this effort yett.

Yes it's good for me.

lib/methods/import.js

ValJed

Just upgrade the stylelint to avoid extra config. Otherwise LGTM.

myovchev · 2024-08-21T12:35:00Z

Just upgrade the stylelint to avoid extra config. Otherwise LGTM.

FYI after stylelint upgrade (still works):

> @apostrophecms/[email protected] stylelint
> stylelint ui/**/*.{scss,vue}

(node:212536) [stylelint:005] DeprecationWarning: `context.fix` is being deprecated.
Please pass a `fix` callback to the `report` utility of "scale-unlimited/declaration-strict-value" instead.
(Use `node --trace-deprecation ...` to show where the warning was created)
(node:212536) [stylelint:005] DeprecationWarning: `context.fix` is being deprecated.
Please pass a `fix` callback to the `report` utility of "scale-unlimited/declaration-strict-value" instead.
(node:212536) [stylelint:005] DeprecationWarning: `context.fix` is being deprecated.
Please pass a `fix` callback to the `report` utility of "scale-unlimited/declaration-strict-value" instead.

Deprecation warnings:
 - The "scss/at-import-partial-extension" rule is deprecated.
 - 'at-import-partial-extension has been deprecated, and will be removed in '7.0'. Use 'load-partial-extension' instead. See: https://github.com/stylelint-scss/stylelint-scss/blob/v6.3.0/src/rules/at-import-partial-extension/README.md

Collect parked and singleton pages as duplicates, record _replaceId

c3df677

myovchev marked this pull request as draft August 19, 2024 10:42

myovchev changed the base branch from main to pro-6387-images-errors August 19, 2024 10:42

Base automatically changed from pro-6387-images-errors to main August 19, 2024 15:50

Update existing singletons, while keeping their ID's in tact

f454400

myovchev commented Aug 19, 2024

View reviewed changes

myovchev requested a review from ValJed August 20, 2024 09:59

myovchev marked this pull request as ready for review August 20, 2024 10:22

myovchev added 6 commits August 20, 2024 13:24

Add dev comments

5fe1865

Merge conflicts

2d923fa

Fix error reporting

8147167

Resolve conflicts

a3795da

Add singleton import option

efef3d7

Revert singleton context menu

1154009

ValJed requested changes Aug 20, 2024

View reviewed changes

myovchev requested a review from ValJed August 20, 2024 16:04

myovchev added 3 commits August 20, 2024 19:50

apos last id for pages

4695135

Manage properly existing page path, rank and level

6ae3548

Refactor and remove debug code

0f95acc

ValJed requested changes Aug 21, 2024

View reviewed changes

myovchev added 4 commits August 21, 2024 12:43

chore: remove branch dep

673242b

More clear pair values

6e9b73a

Remove stylelint disables

85a5faf

Refactor for readibility

52adc9c

myovchev requested a review from ValJed August 21, 2024 10:09

ValJed requested changes Aug 21, 2024

View reviewed changes

chore: stylelint update

3e8e26b

myovchev requested a review from ValJed August 21, 2024 12:34

ValJed approved these changes Aug 21, 2024

View reviewed changes

myovchev merged commit beb3abc into main Aug 22, 2024
6 checks passed

myovchev deleted the pro-6418-parked-singletons branch August 22, 2024 14:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PRO-6418: parked pages & singletons #74

PRO-6418: parked pages & singletons #74

myovchev commented Aug 19, 2024 •

edited

Loading

linear bot commented Aug 19, 2024

myovchev Aug 19, 2024

ValJed left a comment

ValJed Aug 20, 2024

myovchev Aug 20, 2024

ValJed Aug 20, 2024

myovchev Aug 20, 2024

ValJed Aug 21, 2024

myovchev Aug 21, 2024

ValJed Aug 21, 2024

ValJed Aug 21, 2024

myovchev Aug 21, 2024 •

edited

Loading

ValJed Aug 21, 2024

ValJed left a comment

myovchev commented Aug 21, 2024

PRO-6418: parked pages & singletons #74

PRO-6418: parked pages & singletons #74

Conversation

myovchev commented Aug 19, 2024 • edited Loading

Summary

What are the specific steps to test this change?

What kind of change does this PR introduce?

Make sure the PR fulfills these requirements:

linear bot commented Aug 19, 2024

Choose a reason for hiding this comment

ValJed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

myovchev Aug 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ValJed left a comment

Choose a reason for hiding this comment

myovchev commented Aug 21, 2024

myovchev commented Aug 19, 2024 •

edited

Loading

myovchev Aug 21, 2024 •

edited

Loading