handle different locale #48

ETLaurent · 2023-11-20T15:26:44Z

Summary

Import nows detects if the locale found in the exported docs is different from the current site one.

If the site has only one locale configured, then the docs are automatically re-written with the site locale.
If the site has multiple locales configured, the user is given the possibility to abort the import or re-write the docs with the site locale

Note that the verification is done beforehand, meaning that no doc is imported yet when the user is asked about continuing or aborting the import.

Technically, when a locale is found different in the docs, if the site is configured with only one locale then the docs are automatically re-written with req.locale.

However, if multiple locales are configured in the i18n module, then we stop the import and ask the user what we should do.

If he chooses No, the import is stopped and the export docs folder is deleted.
If he chooses Yes, the import route is called again with a parameter forcing the re-writing of the docs locale with req.locale

What are the specific steps to test this change?

On testbed or any a3 project, change the modules/@apostrophecms/i18n/index.js config to either have one or multiple locales.

Export some docs in a locale and re-import them on another locale.

If the site has only one locale configured in i18n, then the docs should be imported in the current site locale.
If the site has multiple locales configured in i18n, then a confirm modal should be displayed. The rest of the test is pretty straightforward, if we choose Yes or No.

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

linear · 2023-11-20T15:26:48Z

PRO-5128 As a user I can import content that was exported from a locale with a different name into the current locale, or decide to cancel

Per discussion with stuart in PRO-5123 this is our interim solution, ready to build and unlock productivity for Michelin's team:

If the locale of the first document in the file does not match the current locale in the UI, and the site we're importing into has more than one locale, then we will display a simple AposModalConfirm confirmation prompt:

"This file was exported from the en-gb locale. Are you sure you want to import it into the en locale?"

If the site has only one locale we will skip this prompt and assume the answer was "Yes."

Note the use of locale names rather than locale labels because we have no idea what the labels were in the exporting site and that's not what matters here anyway, it's the locale name that has to be harmonized.

If the user says no the import is cancelled, if they say yes then the aposLocale and _id properties are remapped on the fly to the current locale so that the import can succeed. Note that relationship fields already use aposDocId which is locale independent and therefore should not be an issue.

Ideally we scan the file until we encounter a document with an aposLocale property (or run out of documents) before displaying this prompt, so clicking "cancel" can do absolutely nothing, but if this is a major implementation issue we could display it the first time we hit a document that does have an aposLocale property and cancel just from that point forward (in other words, somehow backing out imports of localized: false documents reached before that point is not something I want to put on anyone's plate right now, so please do whatever minimizes the scope).

For now we may assume there will only be one locale in the file because we will bump the major version of the export format before we add support for exporting multiple locales. Therefore we only have to check the first document that has an aposLocale property.

Acceptance Criteria

See above novella.

Cypress tests will have to be included in this.

ETLaurent · 2023-11-21T15:52:32Z

lib/methods/import.js

+ }
+ if (notificationId) {
+ self.apos.notification.dismiss(req, notificationId, 2000).catch(self.apos.util.error);
+ }


clean job and notification only if existing

see /api/v1/@apostrophecms/import-export/clean-export call in app.js below, no job nor notification is instantiated at this step, but we still want to remove the export path

ETLaurent · 2023-11-21T17:03:43Z

ui/apos/components/AposDuplicateImportModal.vue

@@ -194,7 +194,7 @@ export default {
 }
 });
 } catch (error) {
- apos.notify(this.$t('aposImportExport:importCleanFailed'), {
+ apos.notify('aposImportExport:importCleanFailed', {


fix interpolation

before after

ell

ETLaurent · 2023-11-21T17:04:27Z

lib/methods/import.js

+ ({ docs, attachmentsInfo } = exportFile);
+ exportPath = exportFile.exportPath;
+
+ await self.cleanFile(exportFile.filePath);


we now clean the archive right after having extracted it so we don't have to think about removing it at several other places

ETLaurent · 2023-11-21T17:05:51Z

lib/methods/import.js

@@ -342,20 +421,20 @@ module.exports = self => {
 }
 },

- async cleanFile(exportPath) {
+ async cleanFile(path) {


minor change: cleanFile also cleans the archive path

ETLaurent · 2023-11-23T16:22:36Z

lib/methods/import.js

+ const differentDocsLocale = self.getFirstDifferentLocale(req, docs);
+ const siteHasMultipleLocales = Object.keys(self.apos.i18n.locales).length > 1;
+
+ // Re-write locale if `overrideLocale` param is passed-on from the import process
+ // (i.e if the user chose "Yes")
+ // or re-write locale automatically on a single-locale site
+ if (differentDocsLocale && (!siteHasMultipleLocales || overrideLocale)) {
+ self.rewriteDocsWithCurrentLocale(req, docs);
+ }
+


we also need to override the locale here!

ETLaurent · 2023-11-23T16:50:23Z

@BoDonkey adding you as a reviewer since I edited the readme ;)

BoDonkey · 2023-11-27T13:04:51Z

CHANGELOG.md

+* Import nows detects if the locale found in the exported docs is different from the current site one. 
+If the site has only one locale configured, then the docs are automatically re-written with the site locale. 
+If the site has multiple locales configured, the user is given the possibility to abort the import or re-write the docs with the site locale.


Suggested change

* Import nows detects if the locale found in the exported docs is different from the current site one.

If the site has only one locale configured, then the docs are automatically re-written with the site locale.

If the site has multiple locales configured, the user is given the possibility to abort the import or re-write the docs with the site locale.

* Import now detects if the locale found in the exported docs is different from the current site one. If the site has only one locale configured, then the docs are automatically re-written with the site locale. If the site has multiple locales configured, the user can abort the import or re-write the docs with the site locale. Note that this requires Apostrophe 3.60.0 or higher for proper functionality.

@BoDonkey removed the whitespaces to make it single line

BoDonkey · 2023-11-27T13:22:54Z

README.md

+Documents will automatically be re-written with the current site locale if only one locale is configured in the `@apostrophecms/i18n` module.
+
+If multiple locales are configured, the user is asked about aborting the import or continuing it:


Suggested change

Documents will automatically be re-written with the current site locale if only one locale is configured in the `@apostrophecms/i18n` module.

If multiple locales are configured, the user is asked about aborting the import or continuing it:

Exported documents maintain their locale settings. If the locale during import differs from the export locale, and only one locale is configured in the @apostrophecms/i18n module, the documents will be automatically rewritten to align with the new import locale.

If multiple locales are set up, the user will be prompted to choose between canceling the import or proceeding with it.

much better, thanks

BoDonkey

Just some documentation changes. Code looks okay to me and works as expected. A "real" engineer might be able to find something that could use improvement. 😃

ValJed · 2023-11-27T14:59:13Z

lib/methods/import.js

+ if (exportPath) {
+ const filesData = await self.getFilesData(exportPath);
+
+ ({ docs, attachmentsInfo } = filesData);


what means the fact that this method receives an exportPath? Does it mean that the gzip file has already been cleaned?

The gzip file is now cleaned as soon as it's extracted (cf. comment below)

This method is provided exportPath when the user chooses Yes (continue the import with the current locale), because is archive has already been extracted during the import method first call.
It just early returned to display the confirmation modal.
So at the 2nd call, we simply give the method back the exportPath so it knows where the files were extracted during the 1st call

ValJed

Few questions, but LGTM

ValJed · 2023-11-27T15:28:51Z

lib/methods/import.js

+
+ // Re-write if user decided to continue the import (`overrideLocale` param sent)
+ // or if the site has only one locale configured
+ self.rewriteDocsWithCurrentLocale(req, docs);


I suppose this method is called twice, another time with the confirmation to change locale?

This method is called only during the 2nd call (when the user chooses Yes), see the early return above

ValJed · 2023-11-27T15:37:34Z

ui/apos/apps/index.js

+ });
+ }
+
+ return;


we agree that in this case the export file is cleaned from the import route directly?

yes, it should be done at line 107, as in a "normal" case where no locale overriding is involved

BoDonkey

Correct the typo, please.

BoDonkey · 2023-11-27T15:50:55Z

CHANGELOG.md

@@ -2,6 +2,12 @@

 ## UNRELEASED

+### Adds
+
+* Import nows detects if the locale found in the exported docs is different from the current site one.


"Import now..."

@BoDonkey done

ETLaurent · 2023-11-27T16:07:11Z

e2e tests run: https://github.com/apostrophecms/testbed/actions/runs/7007618336

BoDonkey

LGTM

wip

c292e80

ETLaurent added 4 commits November 20, 2023 16:44

replace in _id

723cc36

wip

7cdfe74

wip

f8ff7ed

works

d2a22ca

ETLaurent commented Nov 21, 2023

View reviewed changes

ETLaurent added 2 commits November 21, 2023 17:56

clean

52d3ef0

pass raw translation keys in notify to fix interpolating

719f8c5

ETLaurent commented Nov 21, 2023

View reviewed changes

ETLaurent added 2 commits November 22, 2023 12:14

add tests

c0d0df8

add tests

438245b

ETLaurent changed the title ~~wip~~ warn when locale is different Nov 22, 2023

ETLaurent added 2 commits November 23, 2023 11:51

changelog

84786d0

fix single locale site condition and adapt tests

4ced67d

ETLaurent changed the title ~~warn when locale is different~~ handle different locale Nov 23, 2023

ETLaurent added 2 commits November 23, 2023 13:16

add doc

f444864

arr

d2c5306

ETLaurent commented Nov 23, 2023

View reviewed changes

ETLaurent added 2 commits November 23, 2023 17:23

rm only

88c9078

FIX

abf2f59

ETLaurent requested review from ValJed and BoDonkey November 23, 2023 16:49

ETLaurent marked this pull request as ready for review November 23, 2023 16:50

BoDonkey reviewed Nov 27, 2023

View reviewed changes

BoDonkey requested changes Nov 27, 2023

View reviewed changes

ValJed reviewed Nov 27, 2023

View reviewed changes

ValJed approved these changes Nov 27, 2023

View reviewed changes

ETLaurent added 2 commits November 27, 2023 16:43

one line changelog entry

6baee6f

improve readme

72b9512

ETLaurent requested a review from BoDonkey November 27, 2023 15:46

BoDonkey requested changes Nov 27, 2023

View reviewed changes

typo

b74a712

ETLaurent requested a review from BoDonkey November 27, 2023 16:07

add note in changelog

164b8bd

BoDonkey approved these changes Nov 27, 2023

View reviewed changes

ETLaurent merged commit 159bf25 into main Nov 28, 2023
9 checks passed

ETLaurent deleted the pro-5128-different-locale branch November 28, 2023 17:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

handle different locale #48

handle different locale #48

ETLaurent commented Nov 20, 2023 •

edited

Loading

linear bot commented Nov 20, 2023

ETLaurent Nov 21, 2023

ETLaurent Nov 21, 2023

ETLaurent Nov 21, 2023

ETLaurent Nov 21, 2023 •

edited

Loading

ETLaurent Nov 21, 2023

ETLaurent Nov 23, 2023

ETLaurent commented Nov 23, 2023

BoDonkey Nov 27, 2023 •

edited

Loading

ETLaurent Nov 27, 2023

BoDonkey Nov 27, 2023 •

edited

Loading

ETLaurent Nov 27, 2023

BoDonkey left a comment

ValJed Nov 27, 2023

ETLaurent Nov 27, 2023

ValJed left a comment

ValJed Nov 27, 2023

ETLaurent Nov 27, 2023

ValJed Nov 27, 2023

ETLaurent Nov 27, 2023

BoDonkey left a comment

BoDonkey Nov 27, 2023

ETLaurent Nov 27, 2023

ETLaurent commented Nov 27, 2023

BoDonkey left a comment

		Documents will automatically be re-written with the current site locale if only one locale is configured in the `@apostrophecms/i18n` module.

		If multiple locales are configured, the user is asked about aborting the import or continuing it:

+ });
+ }
+ return;

handle different locale #48

handle different locale #48

Conversation

ETLaurent commented Nov 20, 2023 • 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 Nov 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ETLaurent Nov 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ETLaurent commented Nov 23, 2023

BoDonkey Nov 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BoDonkey Nov 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BoDonkey left a comment

Choose a reason for hiding this comment

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

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

BoDonkey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ETLaurent commented Nov 27, 2023

BoDonkey left a comment

Choose a reason for hiding this comment

ETLaurent commented Nov 20, 2023 •

edited

Loading

ETLaurent Nov 21, 2023 •

edited

Loading

BoDonkey Nov 27, 2023 •

edited

Loading

BoDonkey Nov 27, 2023 •

edited

Loading