Skip to content
This repository has been archived by the owner on Sep 5, 2023. It is now read-only.

FRONTEND-8638 :: Feature :: Add Custom Angular Validation Patterns #143

Open
wants to merge 2 commits into
base: feature/frontend-2309-remove-getall-putall
Choose a base branch
from

Conversation

orioljp
Copy link
Contributor

@orioljp orioljp commented Mar 22, 2023

Asana task link:
https://app.asana.com/0/1109863238977521/1204241389098638

Merge / Pull request information:
Two validators have been added, these pattern allow us to use multiple patterns with different ids in the same Angular FormControl. One is to ensure that the pattern is followed and the other uses the pattern to find errors.

@orioljp orioljp requested review from urijp, doup and Alex-DA as code owners March 22, 2023 18:23
Copy link
Contributor

@Alex-DA Alex-DA left a comment

Choose a reason for hiding this comment

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

@orioljp I imagine that you didn't write the code, only moved or copied to a new files. But in general I see to much comments explaining the code and many if/else nested with return's.
You think that it's possible to refactor it in the same PR?.

sql = `${sql} and ${this.deletedAtColumn} is null`;
}
return this.sqlInterface.query(sql, query.ids);
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

@orioljp the else is not needed.

}

if (query instanceof IdsQuery) {
if (values.length !== query.ids.length) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@orioljp double if.

abstract put(value: T | undefined, query: Query): Promise<T>;

public async delete(query: Query): Promise<void> {
if (this.softDeleteEnabled) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@orioljp I see to much if/else with a return's that can be simplified.

Copy link
Contributor

@doup doup left a comment

Choose a reason for hiding this comment

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

I just checked the first file, I don't want to find the relevant changes between the "all" removal changes.

packages/angular/src/form-validation/form-validators.ts Outdated Show resolved Hide resolved
@orioljp
Copy link
Contributor Author

orioljp commented Mar 27, 2023

@Alex-DA I promise I would have been sooooo happy editing the raw-sql.data-source to simplify what we have because as you mentioned, it's a mess. I just made it compliant to our new datasource inteface (no getAll/putAll) because we're not working on a project that worth investing time on improving harmony-nest at this moment, so the code you see is really old. Please reconsider your approval.

@orioljp orioljp changed the base branch from master to feature/frontend-2309-remove-getall-putall April 1, 2023 16:55
@orioljp
Copy link
Contributor Author

orioljp commented Apr 1, 2023

I just checked the first file, I don't want to find the relevant changes between the "all" removal changes.

@doup What you reviewed were the actual changes. I didn't notice that this PR contained the changes from the "all" removal. Thanks for noticing and for you constructive comment

@orioljp
Copy link
Contributor Author

orioljp commented Apr 1, 2023

This PR will remain on hold since the AbstractControl used in this validators is deprecated in Angular 14 and we should upgrade harmony-angular

@lucianosantana
Copy link
Contributor

This PR will remain on hold since the AbstractControl used in this validators is deprecated in Angular 14 and we should upgrade harmony-angular

@orioljp Are you sure it is deprecated on v14? I just checked the API reference and I don't see any mention of this being deprecated: https://v14.angular.io/api/forms/AbstractControl

Same for the next version of Angular : https://next.angular.io/api/forms/AbstractControl#description

@lucianosantana lucianosantana self-requested a review April 7, 2023 16:17
@urijp
Copy link
Contributor

urijp commented Apr 7, 2023

@lucianosantana sorry for not giving a complete explanation. Now forms are generic, so newer versions of angular use an AbstractControl or inherited FormControl and UntypedFormControl, which is an alias of FormControl.

This makes that the current version of harmony that expects an AbstractControl in the validators (without generic) is not compatible with the typed ones used in v14. I always try new harmony versions in SocialPALS and I couldn't make this work because of this incompatibility of types.

@urijp
Copy link
Contributor

urijp commented Apr 7, 2023

@urijp
Copy link
Contributor

urijp commented Apr 7, 2023

Anyway, since Angular 13 will be out of LTS soon and Harmony-Angular is just a small part of this library, I'm thinking on updating the library to Angular v14

@lucianosantana
Copy link
Contributor

Understood. Let's upgrade harmony-angular then.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants