search

genouest / genouestaccountmanager

Account manager for core facility
GNU Affero General Public License v3.0
5 stars 8 forks source link

Project description requirements #468

Closed rsiminel closed 4 months ago

rsiminel commented 4 months ago

Adding front end checks and visual clues for project description requirements (length > 30 chars) BobLamarley already covered back end checks.

closes #462

mboudet commented 4 months ago

Hmm, I think one of the PR already managed that, by setting 'ngNativeValidate', with mean that minlength="30" should be restricting it already. (Unless it doesn't work?)

rsiminel commented 4 months ago

I pulled from the main branch after that PR and in my test env I was getting the "Project description length should at least be 30 char" error that is sent from the back end...

mboudet commented 4 months ago

Ok, I'm guessing that ngNativeValidate set the form as invalid, but we do not check the form state before sending.

Not exactly sure how ngNativeValidate works when used with angular validation to be honest. Might be better to double check on angular side (especially if it does not works).

You might want to check if this.new_project.description exists before checking the .length, it might throw an error otherwise.

mboudet commented 4 months ago

@BobLamarley when you made the PR for requiring 30 characters, did your implementation works on your side? It seems it does not restrict on our side, the query to the backend is sent no matter what

BobLamarley commented 4 months ago

I don't know why it isn't working anymore with ngNativeValide, it was on my branch Anyway, here is my pull request for fixing this : https://github.com/genouest/genouestaccountmanager/pull/469 Instead of using ngNativeValidate who use native HTML 5 form validation, i use angular form validation with ngForm as of now

rsiminel commented 4 months ago

The ng form validation works (but only once you've touched a field). However, when you submit the form, you get a "Project name empty" error sent by the back end. I don't think you can just get rid of the two-way binding of the new_project attributes.

mboudet commented 4 months ago

Ok, so there is now two PR with mostly the same content. Which one should I review?

rsiminel commented 4 months ago

Ok, so there is now two PR with mostly the same content. Which one should I review?

This one. It includes the changes made in Bob's other PR and it works.