Avoid cloning the AutoroutePart pattern in the localized content #15810

hishamco · 2024-04-23T05:56:17Z

Summary by CodeRabbit

New Features
- Enhanced content localization by automatically regenerating permalinks when localizing content.
- Expanded service configurations to support new localization features across content culture pickers and sitemaps.
Refactor
- Introduced new handlers and services for improved content localization management.
Documentation
- Updated project references to support new localization functionalities.

hishamco · 2024-04-23T05:57:16Z

There's might be a better solution, but this the quick one that I go with, @MichaelPetrinolis could you please try the PR

MichaelPetrinolis

If adding a dependency to Autoroute Core is ok, clearing the autoroutepart path does the job. Either it is draftable or not, the localized version has empty path and the user is able to update eg. the localized title and create a localized route. Thanx!

hishamco · 2024-04-23T09:20:57Z

I tried to avoid depending on OC.Autoroute module, but still looking for a better solution if there's

/cc @Skrypt

src/OrchardCore.Modules/OrchardCore.ContentLocalization/DefaultContentLocalizationManager.cs

hishamco · 2024-04-24T01:17:14Z

@coderabbitai review

coderabbitai · 2024-04-24T01:17:22Z

Walkthrough

The update involves improving the content localization process in OrchardCore by automatically regenerating permalinks when content is localized. This enhancement ensures that the autoroute path is correctly handled during localization, addressing a specific issue related to culture names in the path pattern.

Changes

File Path	Change Summary
`.../DefaultContentLocalizationManager.cs`	Added logic to clear `AutoroutePart` path for permalink regeneration during content localization.
`.../OrchardCore.ContentLocalization.csproj`	Added a project reference to `OrchardCore.Autoroute.Core.csproj` to support the new localization functionality.
`.../AutoPartContentLocalizationHandler.cs`	Introduced a new class `AutoPartContentLocalizationHandler` implementing `IContentLocalizationHandler` methods.
`.../Startup.cs`	Various changes including namespace additions and method modifications to support content localization features.

Assessment against linked issues

Objective	Addressed	Explanation
Content Localization Autoroute problem with culture name in path (#15805)	✅	The changes address the issue by enhancing the content localization process to handle autoroute paths correctly.

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>.
- Generate unit testing code for this file.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai generate unit testing code for this file.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai generate interesting stats about this repository and render them as a table.
- @coderabbitai show all the console.log statements in this repository.
- @coderabbitai read src/utils.ts and generate unit testing code.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
- @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai full review to do a full review from scratch and review all the files again.
@coderabbitai summary to regenerate the summary of the PR.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai configuration to show the current CodeRabbit configuration for the repository.
@coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configuration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

hishamco · 2024-04-24T01:20:49Z

Seems coderabbitai ignores my PR :)

Piedone · 2024-04-24T17:29:01Z

Hmm, it seems it didn't actually do a review. I've asked the CR team about this.

hishamco · 2024-05-01T22:06:12Z

@MichaelPetrinolis the new update might much better

coderabbitai

Actionable comments posted: 0

Out of diff range and nitpick comments (1)

src/OrchardCore.Modules/OrchardCore.ContentLocalization/Handlers/AutoPartContentLocalizationHandler.cs (1)

11-19: The logic to clear the AutoroutePart path is correctly implemented to support automatic permalink regeneration.

Consider adding a comment explaining why the path is set to null, for future maintainability.

...dCore.Modules/OrchardCore.ContentLocalization/Handlers/AutoPartContentLocalizationHandler.cs

Co-authored-by: Zoltán Lehóczky <zoltan.lehoczky@lombiq.com>

src/OrchardCore.Modules/OrchardCore.Autoroute/Handlers/AutorouteContentLocalizationHandler.cs

Piedone · 2024-05-02T18:07:30Z

src/OrchardCore.Modules/OrchardCore.Autoroute/Handlers/AutorouteContentLocalizationHandler.cs

This doesn't fix the issue:

2024-05-02_20h06_41.mp4

Please try it without your last suggestion - Using Alter - I'm 100% percentage sure it was working as expected

This is weird I don't know why the

context.ContentItem.Alter<AutoroutePart>(p => p.Path = null);

doesn't work as expected here?!! Do you have any idea or shall I revert the last change

That's good, but please make it work in a statically typed way. We don't need dynamic for this.

I don't know, sorry, I'd also need to get into debugging this to figure it out.

We might need to revise this, using CloningAsync() might resolve the issue but not make sense while LocalizingAsync() should be the suited event for that

if that is the case, then call _session.SaveAsync() after calling LocalizingAsync. But honestly, this sounds like a bad idea to me. IContentHandler should be the right event to hook into at that point. LocalizingAsync should not be used to alter the content item.

LocalizingAsync should not be used to alter the content item.

For that I alert the autoroute path without using Alter() just to solve the above issue

Can you create a test case for the Alter method to make sure STJ isn't causing a problem? Alter should work.

This is #15714 a recent PR that I merged, the Alter was working fine

MikeAlhayek · 2024-06-05T19:18:18Z

@hishamco is this something you want to finish up soon? The longer these PR's stay open the higher the change for conflicts (like the one I just fixed). Also, the longer this take, the less interest from reviewers you'll get in your PRs. I suggest addressing and following up on these PR, otherwise close them.

hishamco · 2024-06-06T00:46:51Z

AFAIK this is ready, but sometimes it happens due the review that's why I mentioned some points in the Discord, in the maintainer channel, seems you are not there yet

Piedone · 2024-06-06T15:34:54Z

This is waiting for you. I commented that the code, which is still the current one doesn't work: https://github.com/OrchardCMS/OrchardCore/pull/15810/files#r1588088481 Mike also had follow-up comments. Once you fix it, please re-request review. There are also build errors now BTW.

hishamco · 2024-06-06T16:21:27Z

There are also build errors now BTW

I just noticed that, seems this happen after merging main

hishamco · 2024-06-17T07:03:51Z

FYI both of Mike's suggestions in the comment don't work, so the current implementation is working as expected

hishamco · 2024-06-17T07:18:19Z

Autoroute.mp4

Piedone · 2024-06-17T15:18:05Z

We still shouldn't have to use dynamic for this. Any changes to the part can cause a compilation error that we won't notice until runtime.

hishamco · 2024-06-17T15:57:41Z

We already use dynamic in many places, BTW I tried all the possible solutions without luck :( feel free to commit if you have a suggestion

Piedone · 2024-06-17T18:32:00Z

Yes, and we shouldn't use dynamic anywhere where better options are available.

hishamco · 2024-06-17T18:46:48Z

So what's the better option here? This PR stays long, let us finalize it before you go into your vacation :)

Piedone · 2024-06-17T19:05:04Z

Alter, as you attempted, figuring out why it didn't work for you.

hishamco · 2024-06-17T19:08:51Z

I already tried many times without luck, and I will try one more time, otherwise let us accept this while it fixes a real issue then we could revise the places that we are using dynamic in the future

github-actions · 2024-08-14T13:22:11Z

This pull request has merge conflicts. Please resolve those before requesting a review.

MikeAlhayek · 2024-08-15T17:43:15Z

for instructions on how to resolve the merge conflicts due to #16572 please follow the step listed in this comment.

github-actions · 2024-10-14T20:40:33Z

It seems that this pull request didn't really move for quite a while. Is this something you'd like to revisit any time soon or should we close? Please comment if you'd like to pick it up.

Avoid cloning the AutoPart pattern in the localized content

a491b55

MichaelPetrinolis approved these changes Apr 23, 2024

View reviewed changes

Piedone requested changes Apr 23, 2024

View reviewed changes

src/OrchardCore.Modules/OrchardCore.ContentLocalization/DefaultContentLocalizationManager.cs Outdated Show resolved Hide resolved

Piedone mentioned this pull request Apr 24, 2024

Set up CodeRabbit for AI code reviews (Lombiq Technologies: OCORE-159) #15813

Merged

This was referenced May 1, 2024

Monthly community metrics report for 2024-04-01..2024-04-30 #15929

Closed

Monthly community metrics report for 2024-04-01..2024-04-30 Emmanjr/OrchardCore#2

Open

hishamco added 3 commits May 2, 2024 00:18

Merge branch 'main' into hishamco/issue-15805

77d190b

Address feedback

4e8b6cb

Merge branch 'main' into hishamco/issue-15805

9abfd72

coderabbitai bot reviewed May 1, 2024

View reviewed changes

Merge branch 'main' into hishamco/issue-15805

96a224d

hishamco requested a review from Piedone May 2, 2024 10:49

Piedone requested changes May 2, 2024

View reviewed changes

hishamco and others added 3 commits May 2, 2024 19:38

Apply suggestions from code review

69793d2

Co-authored-by: Zoltán Lehóczky <zoltan.lehoczky@lombiq.com>

Merge branch 'main' into hishamco/issue-15805

143ec74

Move AutorouteContentLocalizationHandler to Autoroute module

2f6f73b

hishamco commented May 2, 2024

View reviewed changes

src/OrchardCore.Modules/OrchardCore.Autoroute/Handlers/AutorouteContentLocalizationHandler.cs Show resolved Hide resolved

hishamco requested a review from Piedone May 2, 2024 16:49

Piedone requested changes May 2, 2024

View reviewed changes

src/OrchardCore.Modules/OrchardCore.Autoroute/Handlers/AutorouteContentLocalizationHandler.cs Show resolved Hide resolved

Address feedback

11280d1

hishamco requested a review from Piedone May 2, 2024 17:42

Merge branch 'main' into hishamco/issue-15805

62cc6b1

Piedone requested changes May 2, 2024

View reviewed changes

github-actions bot added the merge conflict label Jun 5, 2024

Merge branch 'main' into hishamco/issue-15805

3c33271

github-actions bot removed the merge conflict label Jun 5, 2024

Merge branch 'main' into hishamco/issue-15805

af0ab20

Merge branch 'main' into hishamco/issue-15805

cd4662e

hishamco added 3 commits June 17, 2024 08:51

Merge branch 'main' into hishamco/issue-15805

7965e24

Fix the build

bd997d0

Don't use Alert<TPart>()

6246634

hishamco requested a review from Piedone June 17, 2024 07:20

github-actions bot added the merge conflict label Aug 14, 2024

github-actions bot added the stale label Oct 14, 2024

Merge branch 'main' into hishamco/issue-15805

0703ec2

github-actions bot removed the merge conflict label Oct 14, 2024

hishamco removed the stale label Oct 14, 2024

Merge branch 'main' into hishamco/issue-15805

9f06ec2

Avoid cloning the AutoroutePart pattern in the localized content #15810

Are you sure you want to change the base?

Avoid cloning the AutoroutePart pattern in the localized content #15810

Conversation

hishamco commented Apr 23, 2024 • edited by coderabbitai bot Loading

Summary by CodeRabbit

hishamco commented Apr 23, 2024

MichaelPetrinolis left a comment • edited Loading

Choose a reason for hiding this comment

hishamco commented Apr 23, 2024

hishamco commented Apr 24, 2024

coderabbitai bot commented Apr 24, 2024 • edited Loading

Walkthrough

Changes

Assessment against linked issues

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configuration File (.coderabbit.yaml)

Documentation and Community

hishamco commented Apr 24, 2024

Piedone commented Apr 24, 2024 • edited Loading

hishamco commented May 1, 2024

coderabbitai bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hishamco May 2, 2024 • edited Loading

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

hishamco May 3, 2024 • edited by MikeAlhayek Loading

Choose a reason for hiding this comment

MikeAlhayek commented Jun 5, 2024

hishamco commented Jun 6, 2024

Piedone commented Jun 6, 2024

hishamco commented Jun 6, 2024 • edited Loading

hishamco commented Jun 17, 2024

hishamco commented Jun 17, 2024 • edited Loading

Piedone commented Jun 17, 2024

hishamco commented Jun 17, 2024

Piedone commented Jun 17, 2024

hishamco commented Jun 17, 2024

Piedone commented Jun 17, 2024

hishamco commented Jun 17, 2024

github-actions bot commented Aug 14, 2024

MikeAlhayek commented Aug 15, 2024

github-actions bot commented Oct 14, 2024

hishamco commented Apr 23, 2024 •

edited by coderabbitai bot

Loading

MichaelPetrinolis left a comment •

edited

Loading

coderabbitai bot commented Apr 24, 2024 •

edited

Loading

CodeRabbit Configuration File (`.coderabbit.yaml`)

Piedone commented Apr 24, 2024 •

edited

Loading

hishamco May 2, 2024 •

edited

Loading

hishamco May 3, 2024 •

edited by MikeAlhayek

Loading

hishamco commented Jun 6, 2024 •

edited

Loading

hishamco commented Jun 17, 2024 •

edited

Loading