Admin UI language switcher does not work if the current page's piece-type is present in any global field's joinByOne field

[ericwong3]

Synopsis

Let there be a piece-type with detail page (show.html), let's call it locations. If apostrophe-global contains a joinByOne field with withType set to location, then when an admin/editor is on one of location's detail page, The admin's language switcher will not be able to switch language.

Steps to Reproduce

I have created a demo project at ericwong3/apos-issue-demo. The second and last commit contains everything I modified since init from apos-cli. In short, to setup:

1. Create a new project, install apostrophe-workflow, add workflow config snippet and prefixes settings, added language switcher html to layout.html, create admin account
2. Create a piece-type (I have created two, news and locations, as demo), add some demo HTML to show the piece title
3. In apostrophe-global, add an joinByOne field with withType set to location
4. Start the server, add some pieces for both piece type, export and commit them all in every language. (I added one location called "lololo" and one news called "newnewnew")
5. (Done with parked pages in my demo) Create two list pages for the two piece types.
6. Edit global, pick any location piece you created, export and commit global in every language

Then observe:

• In http://localhost:3000/en/news/newnewnew, both language switcher HTML and admin language switcher works
• In http://localhost:3000/en/locations/lololo, language switcher HTML works but admin language switch does not.

The difference being HTML version has an extra &workflowLocale which the admin UI's link does not.

Investigation

However my finding is more than that, because my HTML language switcher missed the |build({workflowLocale:... portion and failed. Me, without knowing adding workflowLocale would work, tried to debug why the locale query parameter is not working, and thus went down the rabbit hole to the core, and find another cause, which I think is worth to share the finding.

I went to apos-issue-demo/node_modules/apostrophe/lib/modules/apostrophe-pieces-pages/index.js, the self.addUrlsToPieces function, and placed some breakpoints there.

Then I performed curl "http://localhost:3000/modules/apostrophe-workflow/link-to-locale?slug=lololo&locale=fr in another terminal. In this case, self.findForAddUrlsToPieces will be called once with pieceName = "location" while req.locale is the current session's language (or default en).

Next I performed curl "http://localhost:3000/modules/apostrophe-workflow/link-to-locale?slug=newnewnew&locale=fr in another terminal. In this case, self.findForAddUrlsToPieces will be called once with pieceName = "location" while req.locale in the default (en), and then once more with pieceName = "news" while req.locale is the one specified in ?locale=.

This shows that, the issue arises due to the order which modules are executed, i.e. (guessed based on above obsesrvation):

1. apostrophe-global is fetched first, as the location link field got resolved, aposParentPageCache[location] is populated with current locale's location page
2. apostrophe-workflow kicks in, changed req.locale to match ?locale=
3. link-to-locale route controller is executed, and tries to lookup the piece type's parent page. If I am (coincidentally) on a location detail page, then the above cache is used without comparing locale, as a result the determined parent page will be of the current language, and thus language won't switch. On the other hand, for other types, such as news I created for this purpose, language switching will success, because the aposParentPageCache does not contain an entry for news, so now after the req.locale is updated, the redirection will successfully send me to the language specified in locale

Solution

So yeah this is my finding, at first sight adding &workflowLocale= to admin language switcher might be the easiest solution, but I am putting my finding here in case this reveals a greater problem or maybe shed lights to some potential optimization.

For me, since my way of interpreting the issue and code was "oh so aposParentPageCache should have taken locale into account while caching", so I made this patch instead:

diff --git a/node_modules/apostrophe/lib/modules/apostrophe-pieces-pages/index.js b/node_modules/apostrophe/lib/modules/apostrophe-pieces-pages/index.js
index 43367df..aa82be3 100644
--- a/node_modules/apostrophe/lib/modules/apostrophe-pieces-pages/index.js
+++ b/node_modules/apostrophe/lib/modules/apostrophe-pieces-pages/index.js
@@ -394,9 +394,10 @@ module.exports = {
 
     self.addUrlsToPieces = function(req, results, callback) {
       var pieceName = self.pieces.name;
+      var cacheKey = (req.locale || 'default') + '__' + pieceName;
       return async.series({
         getIndexPages: function(callback) {
-          if (req.aposParentPageCache && req.aposParentPageCache[pieceName]) {
+          if (req.aposParentPageCache && req.aposParentPageCache[cacheKey]) {
             return setImmediate(callback);
           }
           return self.findForAddUrlsToPieces(req)
@@ -407,7 +408,7 @@ module.exports = {
               if (!req.aposParentPageCache) {
                 req.aposParentPageCache = {};
               }
-              req.aposParentPageCache[pieceName] = pages;
+              req.aposParentPageCache[cacheKey] = pages;
               return callback(null);
             }
             );
@@ -417,7 +418,7 @@ module.exports = {
           return callback(err);
         }
         _.each(results, function(piece) {
-          var parentPage = self.chooseParentPage(req.aposParentPageCache[pieceName], piece);
+          var parentPage = self.chooseParentPage(req.aposParentPageCache[cacheKey], piece);
           if (parentPage) {
             piece._url = self.buildUrl(parentPage, piece);
             piece._parentUrl = parentPage._url;

PS: Though putting a patch at apostrophe core for workflow (locale) issue is not be entirely appropriate, but this reflects how I approached this problem.

[boutell]

Thank you for the detail. I assume you have checked this in the latest versions of apostrophe and apostrophe-workflow (well, at least workflow 2.27, you don't have to check today's release, it's not relevant)?

On Thu, Nov 21, 2019 at 12:02 PM Eric Wong [email protected] wrote:

Synopsis

Let there be a piece-type with detail page (show.html), let's call it locations. If apostrophe-global contains a joinByOne field with withType set to location, then when an admin/editor is on one of location's detail page, The admin's language switcher will not be able to switch language. Steps to Reproduce

I have created a demo project at ericwong3/apos-issue-demo . In short, to setup:

1. Create a new project, install apostrophe-workflow, add workflow config snippet and prefixes settings, added language switcher html to layout.html, create admin account
2. Create a piece-type (I have created two, news and locations, as demo), add some demo HTML to show the piece title
3. In apostrophe-global, add an joinByOne field with withType set to location
4. Start the server, add some pieces for both piece type, export and commit them all in every language. (I added one location called "lololo" and one news called "newnewnew")
5. (Done with parked pages in my demo) Create two list pages for the two piece types.
6. Edit global, pick any location piece you created, export and commit global in every language

Then observe:

• In http://localhost:3000/en/news/newnewnew, both language switcher HTML and admin language switcher works
• In http://localhost:3000/en/locations/lololo, language switcher HTML works but admin language switch does not. There difference being HTML version has an extra &workflowLocale which the admin UI's link does not.

##Investigation However my finding is more than that, because my HTML language switcher missed the |build({workflowLocale:... portion and failed. Me, without knowing adding workflowLocale would work, tried to debug why the locale query parameter is not working, and thus went down the rabbit hole to the core, and find another cause, which I think is worth to share the finding.

[image: image] https://user-images.githubusercontent.com/1881669/69358048-2cea5e80-0cc1-11ea-95f4-998017b902a1.png

I went to apos-issue-demo/node_modules/apostrophe/lib/modules/apostrophe-pieces-pages/index.js, the self.addUrlsToPieces function, and placed some breakpoints there.

Then I performed curl " http://localhost:3000/modules/apostrophe-workflow/link-to-locale?slug=lololo&locale=fr in another terminal. In this case, self.findForAddUrlsToPieces will be called once with pieceName = "location" while req.locale is the current session's language (or default en).

Next I performed curl " http://localhost:3000/modules/apostrophe-workflow/link-to-locale?slug=newnewnew&locale=fr in another terminal. In this case, self.findForAddUrlsToPieces will be called once with pieceName = "location" while req.locale in the default (en), and then once more with pieceName = "news" while req.locale is the one specified in ?locale=.

This shows that, the issue arises due to the order which modules are executed, i.e. (guessed based on above obsesrvation):

1. apostrophe-global is fetched first, as the location link field got resolved, aposParentPageCache[location] is populated with current locale's location page
2. apostrophe-workflow kicks in, changed req.locale to match ?locale=
3. link-to-locale route controller is executed, and tries to lookup the piece type's parent page. If I am (coincidentally) on a location detail page, then the above cache is used without comparing locale, as a result the determined parent page will be of the current language, and thus language won't switch. On the other hand, for other types, such as news I created for this purpose, language switching will success, because the aposParentPageCache does not contain an entry for news, so now after the req.locale is updated, the redirection will successfully send me to the language specified in locale

##Solution So yeah this is my finding, at first sight adding &workflowLocale= to admin language switcher might be the easiest solution, but I am putting my finding here in case this reveals a greater problem or maybe shed lights to some potential optimization.

For me, since my way of interpreting the issue and code was "oh so aposParentPageCache should have taken locale into account while caching", so I made this patch instead:

diff --git a/node_modules/apostrophe/lib/modules/apostrophe-pieces-pages/index.js b/node_modules/apostrophe/lib/modules/apostrophe-pieces-pages/index.js index 43367df..aa82be3 100644--- a/node_modules/apostrophe/lib/modules/apostrophe-pieces-pages/index.js+++ b/node_modules/apostrophe/lib/modules/apostrophe-pieces-pages/index.js@@ -394,9 +394,10 @@ module.exports = {

 self.addUrlsToPieces = function(req, results, callback) {
   var pieceName = self.pieces.name;+      var cacheKey = (req.locale || 'default') + '__' + pieceName;
   return async.series({
     getIndexPages: function(callback) {-          if (req.aposParentPageCache && req.aposParentPageCache[pieceName]) {+          if (req.aposParentPageCache && req.aposParentPageCache[cacheKey]) {
         return setImmediate(callback);
       }
       return self.findForAddUrlsToPieces(req)@@ -407,7 +408,7 @@ module.exports = {
           if (!req.aposParentPageCache) {
             req.aposParentPageCache = {};
           }-              req.aposParentPageCache[pieceName] = pages;+              req.aposParentPageCache[cacheKey] = pages;
           return callback(null);
         }
         );@@ -417,7 +418,7 @@ module.exports = {
       return callback(err);
     }
     _.each(results, function(piece) {-          var parentPage = self.chooseParentPage(req.aposParentPageCache[pieceName], piece);+          var parentPage = self.chooseParentPage(req.aposParentPageCache[cacheKey], piece);
       if (parentPage) {
         piece._url = self.buildUrl(parentPage, piece);
         piece._parentUrl = parentPage._url;

PS: Though putting a patch at apostrophe core for workflow (locale) issue is not be entirely appropriate, but this reflects how I approached this problem.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/apostrophecms/apostrophe-workflow/issues/288?email_source=notifications&email_token=AAAH27NAHDSRPMXX2S7YIUDQU25IRA5CNFSM4JQFMET2YY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4H3FFVBQ, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAH27LPMYLMS26ZFKGB65DQU25IRANCNFSM4JQFMETQ .

--

THOMAS BOUTELL | CHIEF TECHNOLOGY OFFICER APOSTROPHECMS | apostrophecms.com | he/him/his

[ericwong3]

As I wrote this just now, the package.json says:

    "apostrophe": "^2.47.0",
    "apostrophe-workflow": "^2.28.0"

, and in package lock, apostrophe is on 2.100.1, workflow is on 2.28.0

[boutell]

OK, thanks for checking. Will look into it.

On Thu, Nov 21, 2019 at 12:07 PM Eric Wong [email protected] wrote:

As I wrote this just now, the package.json says:

"apostrophe": "^2.47.0",
"apostrophe-workflow": "^2.28.0"

and in package lock, apostrophe is on 2.100.1, workflow is on 2.28.0

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<https://github.com/apostrophecms/apostrophe-workflow/issues/288?email_source=notifications&email_token=AAAH27K5I4D5Z7BXYRP34O3QU254RA5CNFSM4JQFMET2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEE26RXQ#issuecomment-557181150>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAH27MOZZ43OL3TEHUOOKTQU254RANCNFSM4JQFMETQ>
.

--

THOMAS BOUTELL | CHIEF TECHNOLOGY OFFICER APOSTROPHECMS | apostrophecms.com | he/him/his

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.