-
Notifications
You must be signed in to change notification settings - Fork 113
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
Consistency improvement and minor bug fixes for auth UI pages #857
Conversation
@@ -0,0 +1,12 @@ | |||
export class Regex { |
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.
PR was getting big enough, but probably should rename this class to Constants
so in addition to patterns
could also have:
messages
, for things like "Only lowercase letters, numbers, and hyphens(-) are allowed." and "must be 64 characters or less"numbers
, for things like the64
that is used multiple times inValidators.maxLength(64)
@@ -35,7 +35,7 @@ | |||
<input chefInput formControlName="name" type="text" (keyup)="handleNameChange()"> | |||
</chef-form-field> | |||
</form> | |||
<chef-button primary [disabled]="disableSave || saveInProgress" (click)="saveNameChange()"> | |||
<chef-button primary [disabled]="!updateNameForm.valid || disableSave || saveInProgress" (click)="saveNameChange()"> |
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.
Validator was defined in ts file but not used.
@@ -65,8 +66,7 @@ export class ApiTokenDetailsComponent implements OnInit, OnDestroy { | |||
} | |||
|
|||
public handleNameChange(): void { | |||
this.disableSave = this.updateNameForm.controls['name'].value === this.token.name || | |||
this.updateNameForm.controls['name'].value.trim() === ''; |
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.
This check is now handled automatically by Validator.
@@ -20,8 +21,6 @@ import { | |||
import { CreateToken } from 'app/entities/api-tokens/api-token.actions'; | |||
import { saveStatus, saveError } from 'app/entities/api-tokens/api-token.selectors'; | |||
|
|||
const ID_PATTERN = '[0-9a-z-]+'; |
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.
Magic value--boom!
@@ -72,7 +73,8 @@ export class PolicyAddMembersComponent implements OnInit, OnDestroy { | |||
fb: FormBuilder) { | |||
|
|||
this.expressionForm = fb.group({ | |||
expression: ['', Validators.required] | |||
// Must stay in sync with error checks in policy-add-members.component.html | |||
expression: ['', [Validators.required, Validators.pattern(Regex.patterns.NON_BLANK)]] |
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.
Yes, required
is still needed even if pattern
disallows an empty string.
@@ -18,10 +18,14 @@ <h2>Add Member Expression</h2> | |||
<form [formGroup]="expressionForm"> | |||
<chef-form-field> | |||
<label>Member expression *</label> | |||
<input chefInput formControlName="expression" type="text"> | |||
<input chefInput formControlName="expression" type="text" (keyup)="resetErrors()" > |
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.
As soon as the value is edited, clear the error so it can then appear again if it needs to later.
this.duplicateMember = member.name in this.membersAvailableMap; | ||
if (this.duplicateMember) { | ||
if (member.name in this.membersAvailableMap) { | ||
this.duplicateMember = true; |
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.
Make code parallel to rest of function.
@@ -298,9 +304,6 @@ export class PolicyAddMembersComponent implements OnInit, OnDestroy { | |||
return; | |||
} | |||
}); | |||
if (this.alreadyPolicyMember) { | |||
return; |
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.
Unreachable code
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.
Nitpicks only. thanks!
<chef-error *ngIf="unparsableMember">Invalid member expression.</chef-error> | ||
<chef-error *ngIf="duplicateMember">Member expression already in table.</chef-error> | ||
<chef-error *ngIf="alreadyPolicyMember">Member already in policy.</chef-error> | ||
<chef-error *ngIf="(expressionForm.get('expression').hasError('required') || expressionForm.get('expression').hasError('pattern')) && expressionForm.get('expression').dirty"> |
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.
We don't incude dirty
in the other *ngIf
s -- is that a special thing here?
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.
Yes, it is--meant to add a pre-review comment there. The first three can only ever occur by someone actively pressing "Add", so they cannot show up by themselves on page load, and thus no need of ngIf
. The latter is, like in all the other files, needing of the dirty
flag so that the error does not show up upon page load.
<chef-error *ngIf="(expressionForm.get('expression').hasError('required') || expressionForm.get('expression').hasError('pattern')) && expressionForm.get('expression').dirty"> | ||
Expression is required | ||
</chef-error> | ||
|
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.
[nit] indentation looks off, and this extra line can go
@@ -72,7 +73,8 @@ export class PolicyAddMembersComponent implements OnInit, OnDestroy { | |||
fb: FormBuilder) { | |||
|
|||
this.expressionForm = fb.group({ | |||
expression: ['', Validators.required] | |||
// Must stay in sync with error checks in policy-add-members.component.html |
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.
🤔 Can't we centralize the entire ['', [Validators.required, Validators.pattern(Regex.patterns.NON_BLANK)]]
array, then?
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.
Indeed... another candidate for the Constants file I mentioned above; maybe next PR, though...?
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.
➡️ Can we please get periods at the end of all of our error messages? ⬅️
A non-blank team {{ versionedNameOrId }} is required | ||
</chef-error> | ||
<chef-error *ngIf="teamCreateForm.get('teamId').hasError('required') && teamCreateForm.get('teamId').dirty"> | ||
Team {{ versionedNameOrId }} is required |
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.
so... i think this is fine. but the form labels are Team ID and Name so its a bit weird that the error text uses Team ID and Team name and there is a disconnect there. if we can make those consistent, that would be rad.
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.
Fixed it both for team create and team edit. 👍
<chef-error *ngIf="createUserForm.get('username').hasError('pattern') && createUserForm.get('username').dirty">Invalid | ||
username</chef-error> | ||
<chef-error *ngIf="(createUserForm.get('username').hasError('required') || createUserForm.get('username').hasError('pattern')) && createUserForm.get('username').dirty"> | ||
Invalid username |
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.
can we be more specific here? this error text isn't very helpful...
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.
Split required and pattern to allow giving two specific error messages. 👍
Signed-off-by: michael sorens <msorens@chef.io>
Also added missing 'required' check for username. It turns out that a regex that disallows an empty string is not good enough! That does not trigger a regex failure. Signed-off-by: michael sorens <msorens@chef.io>
Also added missing 'required' check in HTML template. Signed-off-by: michael sorens <msorens@chef.io>
- Delete unreachable code - Parallelize conditionals Signed-off-by: michael sorens <msorens@chef.io>
Signed-off-by: michael sorens <msorens@chef.io>
- The input field actually did disable for an empty input, but presented no error indicator. - Also adding a check for whitespace only being invalid. Signed-off-by: michael sorens <msorens@chef.io>
Signed-off-by: michael sorens <msorens@chef.io>
Doing so, could then remove redundant "manual" check for empty value Signed-off-by: michael sorens <msorens@chef.io>
Ensure: - all error displays do not end in a period - wording the same for same phrases - consistent grammar "A team is..." vs. "Team is...." - note for each Validator identifies which template to sync with - Consistent formatting of <chef-error> elements - "Required" error msg omits the phrase "non-blank" (doesn't add value) Signed-off-by: michael sorens <msorens@chef.io>
Signed-off-by: michael sorens <msorens@chef.io>
Signed-off-by: michael sorens <msorens@chef.io>
Signed-off-by: michael sorens <msorens@chef.io>
A general enhancement in the spirit of this PR though not quite related to the rest of them; I had this lying around for awhile and decided to include it. Signed-off-by: michael sorens <msorens@chef.io>
e1bb740
to
06e9172
Compare
🔩 Description
In the course of acceptance testing, I started noticing some inconsistent wordings, and error triggers. This PR is a general cleanup of a host of little things across policies, projects, users, teams, and tokens (we don't have any forms for roles, so nothing there).
👍 Definition of Done
Cross-resource concerns
Specific resource changes
👟 Demo Script / Repro Steps
⛓️ Related Resources
✅ Checklist