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.

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

Pro 4691 redirect to other locales #18

Merged
merged 12 commits into from
Oct 18, 2023
Merged

Conversation

ValJed
Copy link
Contributor

@ValJed ValJed commented Oct 17, 2023

PRO-4691

Summary

  • Allow to redirect to locales that are not the current one by storing (when the target is an internal page is another locale) the targetLocale in the redirect document. This way we can update the req object based on this information to get the right page relationship, whatever the current locale.

  • Add tests for external redirects, internal redirects, and internal redirects to other locales.

  • Add a query builder and extend the find method to only get redirects that have external redirect or internal redirect to the current locale.
    ⚠️ I don't think this behavior is viable since it could bring BC for users using the find method of the redirect method. Moreover it will feel unnatural for users to not be able to see all redirects in all locales since they aren't localized..
    For this point I think that we should wait and design a way to properly make it work with relationships. We might need that later in another context.
    So I would remove the part that extend the find method and the query builder for now, WDYT?

What are the specific steps to test this change?

You should be able to redirect a page in fr to a page in en using internal redirects (relationship will be retrieved based on its locale).

What kind of change does this PR introduce?

  • Bug fix
  • [] New feature
  • Refactor
  • Documentation
  • Build-related changes
  • Other

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

linear bot commented Oct 17, 2023

PRO-4691 Redirect to other locales (redirect module)

Currently When redirecting to internal page, we make a relationship to this page, if we change the locale the related page is not showed anymore. We started thinking about how to manage relationships in different weird cases.

From Tom:

In A2 we solved this by recording a locale property for them, separately, because we don’t want users to actually click “localize” on a redirect, etc., that’s just silly, but we need to know how to interpret relationships stored in a redirect
And… I think that is appropriate again
When a redirect is saved, the current locale should get stashed on it in a locale property by a beforeSave handler
And then, if it’s an internal redirect, we can use req.clone({ locale: redirect.locale }) to create the req that fetches the internal page.

For an external redirect we wouldn’t need to look at the property and that’s fine."

We could actually use a query builder to hide internal redirects for other locales in the manage view.

That would fix the other problem without waiting for a big rethink of relationship editing.

Acceptance Criteria

You can add a redirect to a page and not just a static URL in the French locale, for example, and then that redirect will work even though French is not the default locale in the project.

If I'm in the English locale, I don't see page redirects that point to pages at all in the list view if they were created in some other locale.

Add functional tests in the module to cover this.

index.js Outdated
find(_super, req, criteria, options) {
return _super(req, criteria, options).withLocaleTarget(req.locale);
}
};
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would not keep this since it might bring weird behavior for users:

  • Why I can't see all my redirects in a locale, they aren't localized?
  • The find method doesn't return all the redirects anymore, why?

@@ -194,6 +219,24 @@ module.exports = {
}
};
},
queries(self, query) {
return {
builders: {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on comment below, I'm not sure this is the right solution here.

@ValJed ValJed requested a review from boutell October 17, 2023 14:50
Copy link
Member

@boutell boutell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do think this is the right approach. We're not in a position time-wise to solve it in a different way right now.

Re: backwards compatibility, the module has been sufficiently broken that I am not concerned. We just need to document the expectations.

However, reading this code over, it seems like you need to move .withLocaleTarget(req.locale) to findForEditing rather than find. Otherwise the find call seen in the middleware will fail when the internal document is in another locale, which is what we were trying to prevent. So, please make that change and verify the cross-locale internal page redirects work even when the target locale differs from the default locale, or whatever the locale is going into the middleware.

This will also mitigate the "surprise effect." You will be able to fetch all redirects regardless of the active locale, unless you are editing them, in which case you should not be able to for the reasons we have discussed.

@ValJed ValJed requested a review from boutell October 18, 2023 09:11
@ValJed
Copy link
Contributor Author

ValJed commented Oct 18, 2023

@boutell Using findForEditing isn't working because it's not called to display the list of pieces in the manager modal. It's only called to load a single piece into the editor modal so it would create a very weird UX (visible but impossible to open), which is not good.
I removes the extended method, and made the query builder by default requesting pieces that have to relationships to other locales. It can be disabled by users if needed, I added a section in the README about redirecting to other locales.
Not sure if it should be a major upgrade.

Copy link
Member

@boutell boutell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Etienne. I do not think a major version bump is needed. Here is why: it didn't work before in these cases, and now it does work for the first time, as long as you switch locales, which is a normal pattern for other content types in Apostrophe. I do not think it will be a problem since we have documented it.

README.md Outdated Show resolved Hide resolved
README.md Outdated
It's possible to redirect from a locale to another one, with external redirections,
since you define manually the url to redirect to.

But also with internal redirects (relationship to page), even if relationships don't support relationships to other locales.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As for internal redirects (relationships with pages), this works across locales as well, but keep in mind that you will only see the internal redirects that target the current locale when managing redirects. To find redirects that target an internal page in a different locale, switch locales before viewing "Manage Redirects."

README.md Outdated Show resolved Hide resolved
@boutell
Copy link
Member

boutell commented Oct 18, 2023

Thanks Val! Geez Tom

@ValJed ValJed merged commit eeb5daa into main Oct 18, 2023
6 checks passed
@ValJed ValJed deleted the pro-4691-redirect-to-other-locales branch October 18, 2023 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants