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

AAE-27343 Listen to form rules changes from reactive widgets #10392

Merged

Conversation

tomgny
Copy link
Contributor

@tomgny tomgny commented Nov 15, 2024

Please check if the PR fulfills these requirements

  • The commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

What kind of change does this PR introduce? (check one with "x")

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation
  • Other... Please describe:

What is the current behaviour? (You can also link to an open issue here)
https://hyland.atlassian.net/browse/AAE-27343

What is the new behaviour?

Does this PR introduce a breaking change? (check one with "x")

  • Yes
  • No

If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...

Other information:

}
}
}
}

private updateReactiveFormControlOnFormRulesEvent(instance: any): void {
instance?.formService.formRulesEvent.pipe(takeUntilDestroyed(this.destroyRef)).subscribe(() => {
instance?.updateReactiveFormControl();
Copy link
Contributor

@BSekula BSekula Nov 15, 2024

Choose a reason for hiding this comment

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

Can we add an interface to instance? what I mean is adding something like:

interface ReactiveFormWidget {
   updateReactiveFormControl: (): void; 
   formService: Type
}

and implement it in reactive widgets

export class DateTimeWidgetComponent extends WidgetComponent implements OnInit, ReactiveFormWidget {..

So updateReactiveFormControl could be easily traced?

Also we can add some comment to this interface, explaining what it is

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point, will add it

Copy link
Contributor

Choose a reason for hiding this comment

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

If you add an interface (or an abstract class like WidgetComponent) please double check with a custom form widget if it's all good

this.dateInputControl.setValue(this.field.value, { emitEvent: false });
}

private updateFormControlState(): void {
this.dateInputControl.setValidators(this.isRequired() ? [Validators.required] : []);
Copy link
Contributor

@BSekula BSekula Nov 15, 2024

Choose a reason for hiding this comment

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

Can we use custom validators in date widget? 🤔 or only required

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as default option we use only required from default validators. for other cases, like min/max constraints, we handle it in the handleErrors method, and its needed to be compatible with existing form structure + other non-reactive form widgets.

@@ -88,14 +92,29 @@ export class FormFieldComponent implements OnInit, OnDestroy {
instance.fieldChanged.subscribe((field) => {
if (field && this.field.form) {
this.visibilityService.refreshVisibility(field.form);
field.form.onFormFieldChanged(field);
this.triggerFormFieldChanged(field);
Copy link
Contributor

Choose a reason for hiding this comment

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

is it worth to have a method just for a single line of code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this method now is used in two places, so thats why i move it to separate method, but i can agree that maybe its not worth

}
});

if (FormFieldTypes.isReactiveType(instance?.field?.type)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I was expecting this to be called when something has changed into the form field so inside the fieldChanged stream

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmmm why? we subscribe to the formRulesEvent when we create this component instance, why do we need to put it under another subscription (nested)? it should listen for these events for the whole lifecycle of the instance

Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need to subscribe to the form rules here, you just need to put your part of the code (the if) into the fieldChanged subscription, otherwise other event relying on triggering a change on the form field will fail to update the validation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if we do not subscribe for formRules changes, we wont be able to update the state of particular instance until the instance.fieldChanged will be triggered (so for example after blur, or input something event).
to make sure I understand you, this is what you mean, right?

Suggested change
if (FormFieldTypes.isReactiveType(instance?.field?.type)) {
if (componentType) {
this.componentRef = this.container.createComponent(componentType);
const instance = this.componentRef.instance;
instance.field = this.field;
instance.fieldChanged.subscribe((field) => {
if (field && this.field.form) {
this.visibilityService.refreshVisibility(field.form);
this.triggerFormFieldChanged(field);
if (FormFieldTypes.isReactiveType(instance?.field?.type)) {
instance?.updateReactiveFormControl();
this.triggerFormFieldChanged(instance.field);
}
}
});

(i tested it and it doesnt work)

}
}
}
}

private updateReactiveFormControlOnFormRulesEvent(instance: any): void {
instance?.formService.formRulesEvent.pipe(takeUntilDestroyed(this.destroyRef)).subscribe(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Reducing the check to only the form rules might miss some other parts where we update the form and the field changed event is triggered

Copy link
Contributor Author

Choose a reason for hiding this comment

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

on each onFieldChanged call, inside the base widget.component.ts , and also on the all events, we update the value of formRulesEvent subject so it will be always triggered for all the events (and its needed for all events, because in modeling, we can set rules for all available event types, like blur, click etc). i think it should be enough, but maybe im not aware of some corner case

@tomgny tomgny marked this pull request as ready for review November 15, 2024 22:33
this.datetimeInputControl.setValue(this.field.value, { emitEvent: false });
}

private updateFormControlState(): void {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
private updateFormControlState(): void {
private updateFormControlValidators(): void {

WDYT?
To me state is really broad term

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 think state in this case better describe what this method does, because it updates also readonly/disabled state (required is also state of the field)

@tomgny tomgny requested review from VitoAlbano and BSekula November 18, 2024 13:01
@VitoAlbano VitoAlbano force-pushed the fix/AAE-27343-listen-to-form-rules-changes-in-reactive-widgets branch from 7da876c to bd2b94d Compare November 18, 2024 13:01
@tomgny tomgny requested a review from wiktord2000 November 18, 2024 13:04
@tomgny tomgny force-pushed the fix/AAE-27343-listen-to-form-rules-changes-in-reactive-widgets branch 2 times, most recently from 82cb763 to 50ad33c Compare November 25, 2024 12:45
@tomgny tomgny force-pushed the fix/AAE-27343-listen-to-form-rules-changes-in-reactive-widgets branch from 50ad33c to e122795 Compare November 28, 2024 08:31
@tomgny tomgny force-pushed the fix/AAE-27343-listen-to-form-rules-changes-in-reactive-widgets branch from e122795 to 7935d17 Compare November 29, 2024 08:37
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
14.7% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

@tomgny tomgny merged commit 23fe4d4 into develop Nov 29, 2024
16 of 17 checks passed
@tomgny tomgny deleted the fix/AAE-27343-listen-to-form-rules-changes-in-reactive-widgets branch November 29, 2024 09:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants