-
Notifications
You must be signed in to change notification settings - Fork 617
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
Form Builder - Move backend to HCMS #3645
base: next
Are you sure you want to change the base?
Conversation
packages/api-form-builder/src/cmsFormBuilderStorage/CmsFormBuilderStorage.ts
Outdated
Show resolved
Hide resolved
6b5be13
to
6e6360c
Compare
@adrians5j, @SvenAlHamad what should we do about these bugs? While working on this feature we have found bug that exists for a long time.
2023-11-08.16.36.41.mov
2023-11-08.17.19.49.mov |
f20f406
to
6e6360c
Compare
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.
Thanks @neatbyte-vnobis and @neatbyte-ivelychko for this Pull Request.
You will find many comments below; most are repetitive, and many others address tiny issues.
There are a couple of issues we need to address before merging:
- we want to keep the original imported method
createFormBuilder
. TLDR: we don't want to upgrade some part of user projects. - we want to keep the GraphQL schema unchanged: some field types have been changed, some of them are not required anymore and vice-versa. Use the API playground in the admin app to confront the newly generated schema with the previous one.
packages/api-form-builder/src/cmsFormBuilderStorage/creteModelField.ts
Outdated
Show resolved
Hide resolved
packages/api-form-builder/src/cmsFormBuilderStorage/CmsFormsStorage.ts
Outdated
Show resolved
Hide resolved
packages/api-form-builder/src/cmsFormBuilderStorage/models/form.model.ts
Outdated
Show resolved
Hide resolved
packages/api-form-builder/src/cmsFormBuilderStorage/models/submission.model.ts
Outdated
Show resolved
Hide resolved
824dfdf
to
eb371aa
Compare
@adrians5j @Pavel910 Any thoughts on this? |
We're still discussing this internally. We'll get back to you re this soon. |
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.
@neatbyte-vnobis, we are almost there 💪.
I highlighted some minor changes, plus while testing the admin app, I noticed a couple of bugs:
- App Form Builder: While trying to create a new version from a previous one, it always keeps the configuration from the latest version instead of the selected one.
- App Page Builder: The application crashes while trying to insert a form on a page. It looks like an error on the
getPublishedForm
query. See the video here below:
CleanShot.2023-12-14.at.07.44.54.mp4
@@ -341,7 +336,7 @@ export const createFormsCrud = (params: CreateFormsCrudParams): FormsCRUD => { | |||
|
|||
if (!original) { | |||
throw new NotFoundError(`Form "${id}" was not found!`); | |||
} else if (original.locked) { | |||
} else if (original.status === "unpublished") { |
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.
Let's create a FORM_STATUS
enum: you can import this enum from api-headless-cms
package and re-export it.
i.e.
if (original.status === CONTENT_ENTRY_STATUS.UNPUBLISHED) {
...
}
@@ -69,7 +69,7 @@ export const createSubmissionsSchema = (params: CreateSubmissionsTypeDefsParams) | |||
* Get all revisions of the form. | |||
*/ | |||
const revisions = await formBuilder.getFormRevisions(form); | |||
const publishedRevisions = revisions.filter(r => r.published); | |||
const publishedRevisions = revisions.filter(r => r.status === "published"); |
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.
Let's create a FORM_STATUS
enum: you can import this enum from api-headless-cms
package and re-export it.
@@ -92,7 +92,7 @@ export const Name = () => { | |||
<NameWrapper> | |||
<FormMeta> | |||
<Typography use={"overline"}>{`status: ${ | |||
state.data.published ? t`published` : t`draft` | |||
state.data.status === "published" ? t`published` : t`draft` |
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.
Let's create an enum with the possible status.
@@ -37,7 +37,7 @@ const revisionsMenu = css({ | |||
|
|||
const getIcon = (revision: Pick<FbFormModel, "status">) => { | |||
switch (revision.status) { | |||
case "locked": | |||
case "unpublished": |
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.
Same here, let's use an enum with the possible status.
@@ -138,7 +138,7 @@ const FormsDataList = (props: FormsDataListProps) => { | |||
const handlerKey = form.id + form.status; | |||
if (!editHandlers.current[handlerKey]) { | |||
editHandlers.current[handlerKey] = async () => { | |||
if (!form.published) { | |||
if (form.status !== "published") { |
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.
Same here, let's use an enum with the possible status.
# Conflicts: # packages/api-headless-cms/src/graphqlFields/json.ts
# Conflicts: # packages/api-form-builder-so-ddb-es/src/index.ts # packages/api-form-builder-so-ddb-es/src/types.ts
At the moment, this PR is on hold because of an issue with using HCMS storage operation for the submission entity. When there is a large number of submissions stored in the database (10,000 submissions), attempting to list even a small subset (20 submissions) results in a This is an example of submission record:
|
Changes
Form Methods moved to HCMS:
Submission Methods moved to HCMS:
Skipped Submission Methods:
deleteSubmission
,getSubmission
. They were skipped because they are not used anywhere.How Has This Been Tested?
Manually.
Documentation
Form Builder - Move backend to HCMS