Client Reset Key UI #4813

chaitali-mane · 2021-03-10T18:09:34Z

🔩 Description: What code changed, and why?

we need to add Client Reset Key functionality on client details page.

⛓️ Related Resources

👍 Definition of Done

I have added Client Reset Key popup on client details page and make reset Key link active.

👟 How to Build and Test the Change

Steps to reproduce the behavior:
if you have sample data for the infra servers than
go to >> infrastructure tab >> click on Chef server on left side navigation bar >> Organizations >> Clients tab >> click on any Client >> details tab >> click on Reset Key

✅ Checklist

I have read the CONTRIBUTING document.
Tests added/updated?
Docs added/updated?
All commits have been signed-off for the Developer Certification of Origin.

📷 Screenshots, if applicable

Client-Reset-Key.mp4

netlify · 2021-03-10T18:09:38Z

Deploy preview for chef-automate processing.

Building with commit b5d5f45

https://app.netlify.com/sites/chef-automate/deploys/6058d8dea5b5b6000771cb52

vivekshankar1

After clicking on reset key the box in the modal where the private key should be, is empty. Also there is an error in console.
I am using progress-chef org in manage.chef.io

chaitali-mane · 2021-03-15T11:09:58Z

After clicking on reset key the box in the modal where the private key should be, is empty. Also there is an error in console.
I am using progress-chef org in manage.chef.io

@vivekshankar1 , Error occured just because of API dependency was not added into master, now It's merged and error not coming. Before merged, we build API locally as mentioned in particular API PR and check UI.

vivekshankar1 · 2021-03-15T13:59:50Z

@chaitali-mane , could you please rebase your branch with the latest master so that backend api code if merged to master will be available in this branch ?

SEAjamieD

So far this is working wonderfully for me with no errors in the console. 🎉

From a purely front-end perspective, I see two things that should really be addressed here.

Its important that anything interactive can be accomplished purely by using the keyboard. There should also be a visual indicator of what button is in focus.
Much of this page is not flexible, and by default we should strive to make every we have flex to any screen. Currently, we aren't concerned with mobile phones, but we should still strive to make the content flex for different browser sizes on a desktop computer.

components/automate-ui/src/app/modules/infra-proxy/client-details/client-details.component.scss

...nts/automate-ui/src/app/modules/infra-proxy/reset-client-key/reset-client-key.component.scss

vivekshankar1 · 2021-03-17T16:39:26Z

Thanks @chaitali-mane , the console error is fixed now and is working as expected.

components/automate-ui/src/app/entities/clients/client-details.reducer.ts

msorens · 2021-03-17T16:31:37Z

components/automate-ui/src/app/entities/clients/client.effects.ts

+    map(({ payload: { name } }: ResetKeyClientSuccess) => {
+      return new CreateNotification({
+        type: Type.info,
+        message: `Successfully resetting Client key - ${name}.`


I see there are a couple instances like you have here, but let's move to be consistent with the majority:

Suggested change

message: `Successfully resetting Client key - ${name}.`

message: `Reset client key ${name}.`

Deleting "Successfully".

Remove hyphen.

Would be helpful if you could fix the other five occurrences of that pattern in the code base.

This format is suggested by UX team, will go with "Successfully resetting Client key - ${name}."

...nts/automate-ui/src/app/modules/infra-proxy/reset-client-key/reset-client-key.component.html

msorens · 2021-03-17T16:36:50Z

.../automate-ui/src/app/modules/infra-proxy/reset-client-key/reset-client-key.component.spec.ts

+    fixture.detectChanges();
+  });
+
+  it('should create', () => {


Need some real unit tests here, too.

I have added more tests for reset key modal.

...nents/automate-ui/src/app/modules/infra-proxy/reset-client-key/reset-client-key.component.ts

msorens · 2021-03-17T16:40:25Z

...nents/automate-ui/src/app/modules/infra-proxy/reset-client-key/reset-client-key.component.ts

+
+  ngOnInit() {
+    this.openEvent.pipe(takeUntil(this.isDestroyed))
+      .subscribe(() => {


Should this block also reset authFailure ?

Code is updated , removed unused code, Please take a look.

msorens · 2021-03-17T16:40:59Z

...nents/automate-ui/src/app/modules/infra-proxy/reset-client-key/reset-client-key.component.ts

+
+  resetKeyClient(): void {
+    this.reseting = true;
+    const payload = {


Please strongly type this constant; avoid the (implicit) any.

I have added type for const payload, Please review it.

...nents/automate-ui/src/app/modules/infra-proxy/reset-client-key/reset-client-key.component.ts

Signed-off-by: chaitali-mane <cmane@progress.com>

…t-key/reset-client-key.component.ts Co-authored-by: Michael Sorens <msorens@chef.io>

…t-key/reset-client-key.component.html Co-authored-by: Michael Sorens <msorens@chef.io>

….reducer.ts Co-authored-by: Michael Sorens <msorens@chef.io>

Signed-off-by: chaitali-mane <cmane@progress.com>

msorens

Looks good; however, should not merge until getting an approval from @SEAjamieD

.../automate-ui/src/app/modules/infra-proxy/reset-client-key/reset-client-key.component.spec.ts

msorens · 2021-03-22T02:46:12Z

...nents/automate-ui/src/app/modules/infra-proxy/reset-client-key/reset-client-key.component.ts

+    ]).pipe(
+      takeUntil(this.isDestroyed))
+      .subscribe(([getStatusSt, resetKeyState, errorSt]) => {
+      if (getStatusSt === EntityStatus.loadingSuccess &&


I have updated indentation. Please have a look.

…t-key/reset-client-key.component.spec.ts Co-authored-by: Michael Sorens <msorens@chef.io>

vivekshankar1

All the suggested changes are made. The ui highlights are working and are keyboard accessible. Also the content is not overflowing out of the containers now. Looks good to me. Thanks @chaitali-mane

Signed-off-by: chaitali-mane <cmane@progress.com>

SEAjamieD · 2021-03-22T17:19:30Z

components/automate-ui/src/app/modules/infra-proxy/client-details/client-details.component.scss

+    chef-button:focus {
+      outline: 0.5px solid $chef-primary-bright;


can remove this line, you've got it covered in the ng-deep below

Removed this code from file. Please check.

SEAjamieD · 2021-03-22T17:20:15Z

components/automate-ui/src/app/modules/infra-proxy/client-details/client-details.component.scss

+
+    ::ng-deep {
+      chef-button button:focus {
+        outline: 0.5px solid $chef-primary-bright;


Suggested change

outline: 0.5px solid $chef-primary-bright;

outline: -webkit-focus-ring-color auto 1px;

This will give us our default focus ring

@SEAjamieD As per UX suggestions, added outline with different colour and different width. Focus changed to 0.5px solid #3864F2

according to your comment below, this may be able to stay as is, since now UX is requesting 0.5px 👍

SEAjamieD · 2021-03-22T17:22:38Z

components/automate-ui/src/app/modules/infra-proxy/client-details/client-details.component.scss

+    chef-icon:focus {
+      outline: none;
+    }
+
+    span:focus {
+      outline: none;
+    }


I don't see where these are doing anything. Looks to me like they can be removed.

yeah, good catch, I removed these line.

SEAjamieD · 2021-03-22T17:28:41Z

...nts/automate-ui/src/app/modules/infra-proxy/reset-client-key/reset-client-key.component.scss

+      chef-button {
+
+        &:focus {
+          outline: 0.5px solid $chef-critical-dark;


I'm noticing that you are using 0.5px for the outlines, is this from the new UX mockups? Currently across our site, all the focus rings are 1px.

I'm asking because I know much of the UX is getting updated, so I want to make sure i'm not missing anything, as 0.5px is used quite a bit in this PR

@SEAjamieD , Yeah it's new design for focus part, UX team suggestions we added 0.5px.

SEAjamieD · 2021-03-22T17:43:41Z

much better flex in the key area now, thank you!

SEAjamieD

I noted a few changes left to make, but all it all, looking good. 🎉

Signed-off-by: chaitali-mane <cmane@progress.com>

chaitali-mane self-assigned this Mar 10, 2021

chaitali-mane requested a review from vinay033 March 10, 2021 18:10

chaitali-mane added automate-infra-proxy automate-ui labels Mar 10, 2021

chaitali-mane marked this pull request as draft March 11, 2021 07:27

kvivek1115 mentioned this pull request Mar 11, 2021

[infra-proxy-service] Reset client added client name in response. #4814

Merged

4 tasks

chaitali-mane changed the title ~~Client Reset Key UI~~ WIP Client Reset Key UI Mar 11, 2021

chaitali-mane changed the title ~~WIP Client Reset Key UI~~ [WIP] Client Reset Key UI Mar 11, 2021

chaitali-mane marked this pull request as ready for review March 12, 2021 12:38

chaitali-mane changed the title ~~[WIP] Client Reset Key UI~~ Client Reset Key UI Mar 12, 2021

vivekshankar1 self-requested a review March 12, 2021 16:19

vivekshankar1 reviewed Mar 12, 2021

View reviewed changes

chaitali-mane force-pushed the Chaitali/client-reset-key branch from ef9c925 to c2bfdcc Compare March 15, 2021 14:14

SEAjamieD closed this Mar 15, 2021

SEAjamieD reopened this Mar 15, 2021

SEAjamieD reviewed Mar 15, 2021

View reviewed changes

components/automate-ui/src/app/modules/infra-proxy/client-details/client-details.component.scss Outdated Show resolved Hide resolved

...nts/automate-ui/src/app/modules/infra-proxy/reset-client-key/reset-client-key.component.scss Outdated Show resolved Hide resolved

chaitali-mane force-pushed the Chaitali/client-reset-key branch from 28d70ea to 790e4a0 Compare March 16, 2021 18:02

himanshi-chhabra mentioned this pull request Mar 17, 2021

Added Create Environment UI #4803

Merged

4 tasks

msorens reviewed Mar 17, 2021

View reviewed changes

chaitali-mane added 7 commits March 19, 2021 15:10

added client reset key

d5fca78

Signed-off-by: chaitali-mane <cmane@progress.com>

updated reset key code

be6a94b

Signed-off-by: chaitali-mane <cmane@progress.com>

unit test issue and style changes

f975731

Signed-off-by: chaitali-mane <cmane@progress.com>

Updated according to API respose changes

b0cff6b

Signed-off-by: chaitali-mane <cmane@progress.com>

minor changes

4873b69

Signed-off-by: chaitali-mane <cmane@progress.com>

Updated Error diplay

bb820e3

Signed-off-by: chaitali-mane <cmane@progress.com>

Update modal Width and focus on button

40caffc

Signed-off-by: chaitali-mane <cmane@progress.com>

chaitali-mane and others added 6 commits March 19, 2021 15:10

Update components/automate-ui/src/app/modules/infra-proxy/reset-clien…

495e81e

…t-key/reset-client-key.component.ts Co-authored-by: Michael Sorens <msorens@chef.io>

Update components/automate-ui/src/app/modules/infra-proxy/reset-clien…

06dc70b

…t-key/reset-client-key.component.ts Co-authored-by: Michael Sorens <msorens@chef.io>

Update components/automate-ui/src/app/modules/infra-proxy/reset-clien…

de95a87

…t-key/reset-client-key.component.html Co-authored-by: Michael Sorens <msorens@chef.io>

Update components/automate-ui/src/app/entities/clients/client-details…

db06304

….reducer.ts Co-authored-by: Michael Sorens <msorens@chef.io>

Updated for review comments

0629732

Signed-off-by: chaitali-mane <cmane@progress.com>

Updated minor changes

c0225c9

Signed-off-by: chaitali-mane <cmane@progress.com>

chaitali-mane force-pushed the Chaitali/client-reset-key branch from e31f3bd to c0225c9 Compare March 19, 2021 09:40

chaitali-mane added 4 commits March 19, 2021 15:35

Updated test name

dfb57b3

Signed-off-by: chaitali-mane <cmane@progress.com>

Lint error removed

c68c065

Signed-off-by: chaitali-mane <cmane@progress.com>

Removed unsed code

9ce462e

Signed-off-by: chaitali-mane <cmane@progress.com>

updated lint error

9c6bfe8

Signed-off-by: chaitali-mane <cmane@progress.com>

msorens approved these changes Mar 22, 2021

View reviewed changes

Update components/automate-ui/src/app/modules/infra-proxy/reset-clien…

f496292

…t-key/reset-client-key.component.spec.ts Co-authored-by: Michael Sorens <msorens@chef.io>

vivekshankar1 approved these changes Mar 22, 2021

View reviewed changes

Suggested change indendation

0476dd3

Signed-off-by: chaitali-mane <cmane@progress.com>

chaitali-mane requested a review from SEAjamieD March 22, 2021 16:05

SEAjamieD reviewed Mar 22, 2021

View reviewed changes

SEAjamieD approved these changes Mar 22, 2021

View reviewed changes

unsed code removed

b5d5f45

Signed-off-by: chaitali-mane <cmane@progress.com>

kalroy merged commit d2dff27 into master Mar 22, 2021

kalroy deleted the Chaitali/client-reset-key branch March 22, 2021 18:45

kalroy added the acceptance: verified label Mar 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Client Reset Key UI #4813

Client Reset Key UI #4813

chaitali-mane commented Mar 10, 2021 •

edited

Loading

netlify bot commented Mar 10, 2021 •

edited

Loading

vivekshankar1 left a comment

chaitali-mane commented Mar 15, 2021

vivekshankar1 commented Mar 15, 2021

SEAjamieD left a comment

vivekshankar1 commented Mar 17, 2021

msorens Mar 17, 2021

chaitali-mane Mar 19, 2021

msorens Mar 17, 2021

chaitali-mane Mar 19, 2021

msorens Mar 17, 2021

chaitali-mane Mar 19, 2021

msorens Mar 17, 2021

chaitali-mane Mar 19, 2021

msorens left a comment

msorens Mar 22, 2021

chaitali-mane Mar 22, 2021

vivekshankar1 left a comment

SEAjamieD Mar 22, 2021

chaitali-mane Mar 22, 2021

SEAjamieD Mar 22, 2021

chaitali-mane Mar 22, 2021

SEAjamieD Mar 22, 2021

SEAjamieD Mar 22, 2021

chaitali-mane Mar 22, 2021

SEAjamieD Mar 22, 2021

chaitali-mane Mar 22, 2021

SEAjamieD Mar 22, 2021

SEAjamieD commented Mar 22, 2021

SEAjamieD left a comment

	message: `Successfully resetting Client key - ${name}.`
	message: `Reset client key ${name}.`

		chef-button:focus {
		outline: 0.5px solid $chef-primary-bright;

	outline: 0.5px solid $chef-primary-bright;
	outline: -webkit-focus-ring-color auto 1px;

Client Reset Key UI #4813

Client Reset Key UI #4813

Conversation

chaitali-mane commented Mar 10, 2021 • edited Loading

🔩 Description: What code changed, and why?

⛓️ Related Resources

👍 Definition of Done

👟 How to Build and Test the Change

✅ Checklist

📷 Screenshots, if applicable

netlify bot commented Mar 10, 2021 • edited Loading

vivekshankar1 left a comment

Choose a reason for hiding this comment

chaitali-mane commented Mar 15, 2021

vivekshankar1 commented Mar 15, 2021

SEAjamieD left a comment

Choose a reason for hiding this comment

vivekshankar1 commented Mar 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msorens left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vivekshankar1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SEAjamieD commented Mar 22, 2021

SEAjamieD left a comment

Choose a reason for hiding this comment

chaitali-mane commented Mar 10, 2021 •

edited

Loading

netlify bot commented Mar 10, 2021 •

edited

Loading