Skip to content

fix: update job creation path defaults and remove Restore option - #241

Open
im-shiv wants to merge 1 commit into
adobe:mainfrom
im-shiv:directory
Open

fix: update job creation path defaults and remove Restore option#241
im-shiv wants to merge 1 commit into
adobe:mainfrom
im-shiv:directory

Conversation

@im-shiv

@im-shiv im-shiv commented May 13, 2026

Copy link
Copy Markdown

sourceRoot/targetRoot filter: hierarchyNotFile → folder (restrict pickers to folders only). targetRoot rootPath: /content/forms/af → /content/dam/formsanddocuments (AF2 fragments land under DAM; align default with actual output location). Remove Restore radio option: unreliable for forms containing fragments since the pre-conversion version does not include converted fragment copies.

sourceRoot/targetRoot filter: hierarchyNotFile → folder (restrict pickers to folders only).
targetRoot rootPath: /content/forms/af → /content/dam/formsanddocuments (AF2 fragments
land under DAM; align default with actual output location).
Remove Restore radio option: unreliable for forms containing fragments since the
pre-conversion version does not include converted fragment copies.
@iamsudhanshu

Copy link
Copy Markdown

remove Restore option
Any reason we are removing this. Ideally this should be fixed - if this is supported by Page migration.

Please log a jira ticket to resolve this.

@iamsudhanshu

Copy link
Copy Markdown

PR #241 — Code Review (though previous comment is still open - why the restore point support is being removed from Forms - instead of a fix):

fix: update job creation path defaults and remove Restore option

Overview

A targeted UI-only change to the Form job creation wizard (form/job/create/.content.xml) with three modifications:

  1. Remove the "Restore" radio button from the Form Handling options.
  2. Change sourceRoot path picker filter from hierarchyNotFile to folder.
  3. Change targetRoot default rootPath from /content/forms/af to /content/dam/formsanddocuments and same filter change.

All three changes are motivated by PR #240's fragment conversion feature.


Issues

1. RESTORE is still reachable via the Full job creator

full/job/create/.content.xml still presents the "Restore" radio option for all content types — including form pages. A user creating a full conversion job that includes forms can still select RESTORE and trigger the broken code path that
doesn't account for converted fragments. The fix is incomplete unless the same RESTORE removal is applied there too.

2. Dead code left in FormConversionJobExecutor

The backend RESTORE handling is never cleaned up:
import static com.adobe.aem.modernize.model.ConversionJob.PageHandling.RESTORE;
// ...
if (pageHandling == RESTORE) {
page = RewriteUtils.restore(pm, page);
}
If RESTORE is intentionally unsupported for forms going forward, this branch should be removed (or replaced with a fast-fail warning). Leaving it makes the unsupported path silently executable via API or the full job wizard.

3. sourceRoot and targetRoot now use different path namespaces — no UI hint

After this change, sourceRoot browses under /content/forms/af (AF page tree) while targetRoot browses under /content/dam/formsanddocuments (DAM asset tree). These are two different views of the same logical structure, but there's nothing in
the UI (field labels, descriptions, or fieldDescription attributes) to tell the user why the target lives under DAM while the source lives under AF. Users familiar with the old behavior will be confused. A fieldDescription on targetRoot
explaining the DAM root convention would help.

4. filter="folder" may be too restrictive for sourceRoot

hierarchyNotFile allows selecting any non-file hierarchy node (cq:Page, sling:Folder, nt:unstructured). folder typically matches only sling:Folder and nt:folder nodes. Under /content/forms/af, organizational nodes are often sling:Folder, but
projects created in older AEM versions may use nt:unstructured or even cq:Page nodes as folder containers. Changing to folder could silently prevent valid source roots from being selectable. The same concern applies to targetRoot under
/content/dam/formsanddocuments — DAM folder nodes use sling:OrderedFolder or dam:AssetFolder, which may or may not match the folder filter depending on the AEM version. This should be verified against the Granite pathfield filter contract for
the target AEM version.

5. "Add Forms" picker at line 186 still uses hierarchyNotFile

The scope step's picker (used to actually select individual forms to add to the job) was not changed — still filter=hierarchyNotFile. That's correct behavior, but it creates an internal inconsistency in the file: the root path pickers use
folder, the form selection picker uses hierarchyNotFile. A comment or at least consistency in the future would help maintainability.

6. No CHANGELOG entry

Removing the Restore option is a user-visible behavioral change. The CHANGELOG.md has an [Unreleased] section but this PR adds nothing to it. Users upgrading and relying on Restore for forms won't have a migration path documented.

@bstopp bstopp left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

See PR review from Claude, provided by @iamsudhanshu

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.

3 participants