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(cloud): allow setting custom form key #592

Merged
merged 1 commit into from
Feb 25, 2022

Conversation

smbea
Copy link
Contributor

@smbea smbea commented Feb 23, 2022

@bpmn-io-tasks bpmn-io-tasks bot added the needs review Review pending label Feb 23, 2022
@smbea smbea self-assigned this Feb 23, 2022
@smbea smbea requested review from a team, Skaiir and MaxTru and removed request for a team February 23, 2022 08:58
Copy link
Contributor

@MaxTru MaxTru left a comment

Choose a reason for hiding this comment

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

I can break this when using undo:

  1. Select Camunda Forms
  2. Do Undo
  3. => weird state
    broken

@MaxTru MaxTru added the in progress Currently worked on label Feb 23, 2022 — with bpmn-io-tasks
@MaxTru MaxTru removed the needs review Review pending label Feb 23, 2022
@smbea smbea force-pushed the allow-setting-form-key-cloud branch 2 times, most recently from 4f45d11 to 5b31cec Compare February 23, 2022 14:45
@smbea smbea requested a review from MaxTru February 23, 2022 14:53
@smbea smbea added the needs review Review pending label Feb 23, 2022 — with bpmn-io-tasks
@smbea smbea removed the in progress Currently worked on label Feb 23, 2022
@smbea
Copy link
Contributor Author

smbea commented Feb 23, 2022

It was a small issue with the commands executed with the command stack -> fixed

@smbea smbea assigned smbea and unassigned smbea Feb 23, 2022
Copy link
Contributor

@MaxTru MaxTru left a comment

Choose a reason for hiding this comment

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

For all test cases I miss a should react to external changes (e.g. like undo). This should hvae then also catched the bug I reported before.

src/provider/zeebe/properties/FormProps.js Outdated Show resolved Hide resolved
src/provider/zeebe/properties/FormProps.js Outdated Show resolved Hide resolved
id: 'formType',
component: FormType,
isEdited: isSelectEntryEdited
},
Copy link
Contributor

Choose a reason for hiding this comment

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

You always push the two input components, and they then may return undefined if no applicable.

Would it not make more sense to check whether they are applicable already here?

E.g., have if (isCamundaForm(element, formHelper)) directly here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did this because I got an error when trying to do injector.invoke(FormHelper) in the main component FormProps: unhandled error in event listener TypeError: Cannot read properties of null (reading 'invoke'). Do you know why this might happen?

Copy link
Member

Choose a reason for hiding this comment

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

injector is not defined, that is what the error says.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But I'm trying to do:

const injector = useService('injector');
const formHelper = injector.invoke(FormHelper);

Like I do in the other components, and only on the main one I get that error.

Copy link
Member

Choose a reason for hiding this comment

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

Could you check-in a failing test case? I'm happy to have a look.

Copy link
Contributor Author

@smbea smbea Feb 24, 2022

Choose a reason for hiding this comment

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

I added a test branch with these changes). More specifically:

const formHelper = injector.invoke(FormHelper);

This causes a bunch of tests to fail because it throws the error I mentioned.

@MaxTru MaxTru added the in progress Currently worked on label Feb 23, 2022 — with bpmn-io-tasks
@MaxTru MaxTru removed the needs review Review pending label Feb 23, 2022
@smbea smbea force-pushed the allow-setting-form-key-cloud branch from 5b31cec to 53b97be Compare February 24, 2022 12:34
@smbea smbea added the needs review Review pending label Feb 24, 2022 — with bpmn-io-tasks
@smbea smbea removed the in progress Currently worked on label Feb 24, 2022
@smbea smbea requested a review from MaxTru February 24, 2022 12:37
@smbea smbea force-pushed the allow-setting-form-key-cloud branch 2 times, most recently from e57a770 to bce4b47 Compare February 24, 2022 14:28
@smbea smbea added in progress Currently worked on and removed needs review Review pending labels Feb 24, 2022
@smbea smbea force-pushed the allow-setting-form-key-cloud branch from bce4b47 to 1e66811 Compare February 25, 2022 14:02
@smbea smbea added the needs review Review pending label Feb 25, 2022 — with bpmn-io-tasks
@smbea smbea removed the in progress Currently worked on label Feb 25, 2022
@smbea
Copy link
Contributor Author

smbea commented Feb 25, 2022

I was able to figure out the issue with the injector and formHelper. Reopened for review (@MaxTru)

@smbea smbea requested a review from nikku February 25, 2022 14:07
Copy link
Contributor

@MaxTru MaxTru left a comment

Choose a reason for hiding this comment

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

👏

@fake-join fake-join bot merged commit ed07338 into master Feb 25, 2022
@fake-join fake-join bot deleted the allow-setting-form-key-cloud branch February 25, 2022 14:50
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Feb 25, 2022
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