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

Feat/migrate pages to content hub #281

Merged
merged 4 commits into from
Nov 8, 2024
Merged

Conversation

tkrch
Copy link
Collaborator

@tkrch tkrch commented Oct 29, 2024

Motivation

Continuation of #260 , #278

@farmergeek94
Copy link

@tkrch While this is working good, there is one extra bit of info that is still coming in. The content types are still getting assigned to the channel via the table [CMS_ContentTypeChannel]. This allows them to still be added as web page items on the imported channel.

@bluemodus-mwills
Copy link

Hi @tkrch & @farmergeek94 -- @twozero6, @bluemodus-gfuller , and I are working on a migration right now, and would like to use this feature. Is it safe for us to work off this branch, or should we wait? Does the time it's taking on review mean there are known issues or changes are coming?

@tkrch
Copy link
Collaborator Author

tkrch commented Nov 6, 2024

Hi @bluemodus-mwills, yes some changes will be there, but not huge ones in terms how feature will be used. you can use this branch as a base. I will rebase it to master today.

@tkrch tkrch force-pushed the feat/migrate_pages_to_content_hub branch from 94b781c to 78cf3a6 Compare November 6, 2024 16:32
@tkrch
Copy link
Collaborator Author

tkrch commented Nov 6, 2024

@bluemodus-mwills rebased onto master branch, i will speed-up remaining work in order to deliver fully functioning feature.

@bluemodus-mwills
Copy link

Thank you, @tkrch !

@tkrch tkrch requested a review from akfakmot November 6, 2024 23:38
@tkrch tkrch force-pushed the feat/migrate_pages_to_content_hub branch from 3e553db to 5818f39 Compare November 6, 2024 23:47
@tkrch tkrch force-pushed the feat/migrate_pages_to_content_hub branch from 4bcfcec to 8443770 Compare November 6, 2024 23:54
@tkrch
Copy link
Collaborator Author

tkrch commented Nov 6, 2024

Hi @bluemodus-mwills, @twozero6, @bluemodus-gfuller I have updated solution, interface (remodelling sample is unchanged). Only change from perspective of usage is removal of ConvertClassesToContentHub appsettings.json option (the other option CreateReusableFieldSchemaForClasses will be removed soon).

Sample AddReusableRemodelingSample stays unchanged and can be used to configure conversion correctly.

There is one thing that not directly affects feature and that is - what to do with child pages (if any present) under converted page. (i will raise this for further planning)

EDIT: use this branch, but hopefully PR will be soon accepted

@farmergeek94
Copy link

farmergeek94 commented Nov 7, 2024

@tkrch does the latest change force you to handle the entire mapping manually? It was really nice to just specify a config entry and have everything map one to one without needing to set up all of the field mappings.

We are also using this currently for a migration. Hence the question.

@tkrch
Copy link
Collaborator Author

tkrch commented Nov 7, 2024

@farmergeek94 yes, the setting was dropped in favour of mapping. Those 2 features were conflicting with each other, but if requested, it might return as form of "automapping" that would be configured through appsettings.json.

A more universal approach needs to taken, because there are multiple source types for conversion to content hub (custom classes, custom table or maybe basically anything if I implement mapping ability to load data).

@farmergeek94 I will re-introduce this setting next day back into this PR (for not breaking Your current process), but it will be replaced in future.

@bluemodus-mwills
Copy link

Thank you @tkrch !

@farmergeek94
Copy link

Ok that makes sense. We can always fork that feature branch if we have to as well.

@twozero6
Copy link

twozero6 commented Nov 7, 2024

@akfakmot I've been working in this branch and I ran into a bit of an issue.

{72FFD4EB-B7FD-44A0-8B68-BB23984A4101}

When I hit line 227, f is null and throws an exception. Even if I swap the fields around, it's always the last field that has the issue. I did a bit of digging and found that the form definition on line 139 in MigratePageTypesCommandHandler.cs is always missing the last field. It also has some unexpected things as if the form definition from the source was merged with the new one. I did make sure that my field names were unique. Is this something you could look into?

Also, I can start a new issue for this if you think it's appropriate. Is it possible to get the Node context in the ConvertFrom method? For example, I would like to get the node's NodeGUID so that I can do additional lookups (get children) before setting the value.

@tkrch
Copy link
Collaborator Author

tkrch commented Nov 7, 2024

Hi @twozero6, i will look into it. From what I can see - You use for fields LinkMetaTextzzz and LinkMetaAriazzz same source field LinkText2. That might be the issue, i will check if it is supported (If not not, I will add support).

@tkrch
Copy link
Collaborator Author

tkrch commented Nov 7, 2024

@twozero6 that's the issue, fix is on the way

@tkrch
Copy link
Collaborator Author

tkrch commented Nov 7, 2024

@twozero6 field cloning introduced, let me know if feature works for You and in Your case

@tkrch
Copy link
Collaborator Author

tkrch commented Nov 7, 2024

@farmergeek94 requested feature re-introduced with handling of edge-cases. Please note, that in future, feature ConvertClassesToContentHub will be replaced with more general approach.

@tkrch
Copy link
Collaborator Author

tkrch commented Nov 7, 2024

@bluemodus-mwills, @bluemodus-gfuller feature is hopefully complete, if You have any feedback please share. Thanks TK

@tkrch tkrch requested a review from akfakmot November 7, 2024 21:15
@twozero6
Copy link

twozero6 commented Nov 8, 2024

@tkrch the fix worked and I was able to import everything successfully. Thank you for the extremely fast turnaround!

@tkrch tkrch merged commit a9cb03a into master Nov 8, 2024
2 checks passed
@tkrch tkrch deleted the feat/migrate_pages_to_content_hub branch November 8, 2024 13:06
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.

6 participants