-
Notifications
You must be signed in to change notification settings - Fork 8
Bugfix/cmg 616 #673
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
Bugfix/cmg 616 #673
Conversation
…hen for existing case it is getting mapped to title of existing content type
…r into bugfix/beta-release
…r into bugFix/beta-release
…r into bugFix/beta-release
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements project creation navigation flow, enhances locale-mapping validation logic, and refines various utility and mapping functions for migration.
- Add
createProjectCallin Projects page to dispatch default migration data and navigate to migration steps. - Tighten locale mapping checks by using
Object.entriesand update interface toRecord<string, string>. - Fix field-attacher to include
contentTypeId, improve merging logic in content-type creation, and dedupe WordPress post titles.
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| ui/src/pages/Projects/index.tsx | Added createProjectCall, useNavigate, dispatch default migration data, and corrected blocking logic |
| ui/src/components/Modal/modal.interface.ts | Updated ProjectModalProps to accept createProject callback |
| api/src/utils/field-attacher.utils.ts | Included contentTypeId in .find query |
| api/src/utils/content-type-creator.utils.ts | Refactored mergeArrays, mergeFields, and mergeTwoCts; added keyMapper |
| ui/src/components/ContentMapper/index.scss | Added active filter styling |
Comments suppressed due to low confidence (2)
ui/src/pages/Projects/index.tsx:138
- The expression
isModalOpen || truealways evaluates totrue, so navigation is permanently blocked. Remove the|| trueto only block when the modal is actually open.
useBlockNavigation(isModalOpen || true);
api/src/utils/field-attacher.utils.ts:25
- The variable
contentIdis not defined in this scope. It should likely becontentTypeIdto match the function parameter.
.find({ id: fieldUid, projectId: projectId , contentTypeId: contentId})
| selectedOrg: IDropDown; | ||
| closeModal: () => void; | ||
| isOpen: (flag: boolean) => void; | ||
| createProject: (values : FormData)=> void |
Copilot
AI
May 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The createProject prop is declared to return void, but the implementation returns a Promise with a result. Update the interface to something like Promise<any> (or a more specific type) to match its actual return value.
| font-size: $size-font-large; | ||
| font-weight: $font-weight-semi-bold; | ||
| } | ||
| .filtetButton-color{ |
Copilot
AI
May 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The class name filtetButton-color appears to have a typo. Consider renaming it to filterButton-color for clearer intent.
| .filtetButton-color{ | |
| .filterButton-color{ |
| },[newMigrationData?.isprojectMapped]); | ||
|
|
||
| useEffect(()=>{ | ||
| if(! isEmptyString(newMigrationData?.destination_stack?.selectedStack?.value ) |
Copilot
AI
May 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This effect references newMigrationData.destination_stack.selectedStack.value but only lists newMigrationData in its dependency array. Consider adding the specific nested property to the array to prevent stale reads or unintended re-renders.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes issues related to project migration and updates various components to use newly introduced APIs and updated field identifiers for consistency. Key changes include the addition of a createProjectCall in the Projects page, enhanced locale mapping validation in the Migration page, and widespread modifications in ContentMapper and AdvancePropertise to use backupFieldUid for improved field matching.
Reviewed Changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| ui/src/pages/Projects/index.tsx | Adds createProjectCall and integrates navigation and redux update for migration flow. |
| ui/src/pages/Migration/index.tsx | Updates localeMapping validation using Object.entries and fallback object. |
| ui/src/context/app/app.interface.ts | Refines localeMapping type to a Record<string, string>. |
| ui/src/components/Modal/* | Updates Modal interface/implementation to use createProject prop for project creation. |
| ui/src/components/ContentMapper/* | Updates various usages from uid to backupFieldUid to ensure field consistency. |
| ui/src/components/AdvancePropertise/* | Passes contentstackFieldUid through to updateFieldSettings calls. |
| api/src/utils/* & api/src/services/* | Minor modifications to API utilities and services addressing field attacher and migration logging. |
| .talismanrc | Adds a new checksum entry for ContentMapper file. |
Comments suppressed due to low confidence (1)
api/src/services/wordpress.service.ts:2334
- [nitpick] Using a Map to track seen titles is effective; however, if the number of posts is extremely large, consider benchmarking this approach to ensure it scales efficiently.
const seenTitles = new Map();
| return res?.error; | ||
| } | ||
| if (res?.status === 201) { | ||
| const projectId = res?.data?.project?.id; | ||
| dispatch(updateNewMigrationData(DEFAULT_NEW_MIGRATION)) | ||
| navigate(`/projects/${projectId}/migration/steps/1`); | ||
|
|
||
| } | ||
| return res; | ||
|
|
Copilot
AI
Jun 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function returns res.error when an error is encountered, which may not conform to the CreateProjectResponse type. Consider standardizing the return value for error cases for consistency in response handling.
| return res?.error; | |
| } | |
| if (res?.status === 201) { | |
| const projectId = res?.data?.project?.id; | |
| dispatch(updateNewMigrationData(DEFAULT_NEW_MIGRATION)) | |
| navigate(`/projects/${projectId}/migration/steps/1`); | |
| } | |
| return res; | |
| return { | |
| status: res?.status || 500, | |
| error: res?.error, | |
| data: null | |
| } as CreateProjectResponse; | |
| } | |
| if (res?.status === 201) { | |
| const projectId = res?.data?.project?.id; | |
| dispatch(updateNewMigrationData(DEFAULT_NEW_MIGRATION)) | |
| navigate(`/projects/${projectId}/migration/steps/1`); | |
| } | |
| return res; |
| setIsLoading(true); | ||
|
|
||
| const hasNonEmptyMapping = | ||
| newMigrationData?.destination_stack?.localeMapping && |
Copilot
AI
Jun 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The updated localeMapping validation uses a fallback empty object and Object.entries, which improves robustness but may benefit from a brief inline comment describing the intent. Adding such documentation can help future maintainers understand the check.
| newMigrationData?.destination_stack?.localeMapping && | |
| newMigrationData?.destination_stack?.localeMapping && | |
| // Use a fallback empty object to ensure Object.entries does not throw an error if localeMapping is null or undefined. | |
| // Validate that each entry in localeMapping has a non-empty label and a defined, non-empty value. |
| postdataCombined = { ...postdataCombined, ...chunkPostData }; | ||
|
|
||
| const seenTitles = new Map(); | ||
| Object?.entries(postdataCombined)?.forEach(([uid, item]:any) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Object?.entries?.(postdataCombined)?.forEach?.(([uid, item]:any)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| Object?.entries(postdataCombined)?.forEach(([uid, item]:any) => { | ||
| const originalTitle = item?.title; | ||
|
|
||
| if (seenTitles.has(originalTitle)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| if (item?.contentstackFieldType === 'group') { | ||
| const groupSchema: any = { ...item, schema: [] } | ||
| if (item?.contentstackFieldUid?.includes('.')) { | ||
| const parts = item?.contentstackFieldUid?.split('.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why we are making changes in this file @AishDani
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If any group from the source CMS is mapped to a nested group present in Contentstack, then we are modifying its Contentstack UID with all nested groups by adding a dot in between. In this function we are arranging schema of group so keeping only nested group uid to source group which will help to find match in Contentstack schema.
| if (et?.contentstackFieldUid?.includes(`${item?.contentstackFieldUid}.`) || | ||
| (newStack === false && et?.uid?.includes(`${item?.uid}.`))) { | ||
| const target = groupSchema?.contentstackFieldUid; | ||
| const index = et?.contentstackFieldUid?.indexOf(target); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
et?.contentstackFieldUid?.indexOf?.(target)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| // const validLabels = cmsLocaleOptions?.map((item)=> item?.label); | ||
|
|
||
| const existingMasterKey = Object?.keys(selectedMappings || {})?.find((key) => | ||
| const existingMasterID = Object?.keys(selectedMappings || {})?.find((key) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Object?.keys?.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| const existingLabel = existingMasterID; | ||
| const expectedLabel = `${locale?.label}-master_locale`; | ||
|
|
||
| const isLabelMismatch = existingLabel && existingLabel.localeCompare(expectedLabel) !== 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
existingLabel?.localeCompare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| ) => { | ||
| const selectedLocaleKey = selectedValue?.value; | ||
|
|
||
| let existingLabel = existingField[index]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
existingField?.[index]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
No description provided.