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.

Sign up for GitHub

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

Jump to bottom

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

Open
ericwong3 opened this issue Nov 21, 2019 · 4 comments
Open

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

ericwong3 opened this issue Nov 21, 2019 · 4 comments
Labels
bug

Comments

@ericwong3
Copy link

ericwong3 commented Nov 21, 2019 •
edited
Loading

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.

image

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
Copy link
Contributor

boutell commented Nov 21, 2019 via email

@ericwong3
Copy link
Author

ericwong3 commented Nov 21, 2019 •
edited
Loading

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
Copy link
Contributor

boutell commented Nov 21, 2019 via email

@stale
Copy link

stale bot commented Jun 19, 2020

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.

@stale stale bot added the stale label Jun 19, 2020
@abea abea added the bug label Jun 22, 2020
@stale stale bot removed the stale label Jun 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@boutell @abea @ericwong3