Added consistency improvement in list, details and create page of node credentials. #4323

samshinde · 2020-09-09T08:57:18Z

Signed-off-by: samshinde ashinde@chef.io

🔩 Description: What code changed, and why?

consistency improvements for the list, details and create of node credential.

create node credential using the modal popup
Delete node credential modal pop-up with notification
Details tab which has update and reset buttons of each type(ssh, winrm, sudo).
ssh has a dropdown for password or rsa key, since only 1 is required not both

⛓️ Related Resources

👍 Definition of Done

I have added consistency changes for the create Node Credential , list, update and delete Node Credential with UI changes.

👟 How to Build and Test the Change

visit the Settings >> Node Credential page,
create Node Credential - Create node credential according to the type you need.
delete Node Credential - click to any Node Credential left side three-dot menu ... where you can see the delete Node Credential link to test delete the Node Credential.

STEP 1
inside the hab studio

[default:/src:0]# build components/automate-ui-devproxy/
[default:/src:0]# start_automate_ui_background
[default:/src:0]# start_all_services

STEP 2
open new window
go to automate UI path

$ cd components/automate-ui
and run the command

✅ 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

netlify · 2020-09-09T08:57:23Z

Deploy preview for chef-automate ready!

Built with commit aeb328e

https://deploy-preview-4323--chef-automate.netlify.app

...e/+node-credentials/create-node-credential-modal/create-node-credential-modal.component.html

components/automate-ui/src/app/entities/node-credentials/node-credential.effects.ts

...e/+node-credentials/create-node-credential-modal/create-node-credential-modal.component.html

susanev · 2020-09-15T15:59:41Z

@samshinde both the details and reset credential forms have issues with the spacing between the inputs, please use the same amount of space as the other forms, i believe its 30px

...e/+node-credentials/create-node-credential-modal/create-node-credential-modal.component.html

samshinde · 2020-09-16T07:59:21Z

@samshinde both the details and reset credential forms have issues with the spacing between the inputs, please use the same amount of space as the other forms, I believe its 30px

@susanev I have updated the space between the inputs for the details and reset the credential tab.

msorens

Some initial feedback; more later...

components/automate-ui/src/app/app-routing.module.ts

components/automate-ui/src/app/entities/node-credentials/node-credential.actions.ts

components/automate-ui/src/app/entities/node-credentials/node-credential.effects.ts

components/automate-ui/src/app/entities/node-credentials/node-credential.model.ts

...s/+compliance/+node-credentials/node-credential-details/node-credential-details.component.ts

components/automate-ui/src/app/entities/node-credentials/node-credential.model.ts

msorens

A lot of good work here!
Note that I have not exercised the code; my notes are from code-reading only.

components/automate-ui/src/app/entities/node-credentials/node-credential.reducer.ts

components/automate-ui/src/app/entities/node-credentials/node-credential.selectors.ts

...e/+node-credentials/create-node-credential-modal/create-node-credential-modal.component.html

.../pages/+compliance/+node-credentials/node-credentials-list/node-credential-list.component.ts

components/automate-ui/src/app/pages/+compliance/+node-credentials/node-credentials.module.ts

...ages/+compliance/+node-credentials/node-credentials-list/node-credential-list.component.html

msorens

Thanks for the updates; did another review pass with some additional notes.

...e/+node-credentials/create-node-credential-modal/create-node-credential-modal.component.html

...+compliance/+node-credentials/node-credential-details/node-credential-details.component.html

...e/+node-credentials/create-node-credential-modal/create-node-credential-modal.component.html

msorens · 2020-09-24T00:19:08Z

...s/+compliance/+node-credentials/node-credential-details/node-credential-details.component.ts

+          this.resetSuccessful = (state === EntityStatus.loadingSuccess);
+          this.resetForm.markAsPristine();
+        }
+        this.getNodeCred();


Delete this--method is already called on L84 and should only be called once.

@msorens Actually we don't have any response after calling the patch Api so for updating the node credential we need to call it as the same happening in creating success.

No, this violates subscription lifecycle management. The reason I asked you even to rename it to setupNodeSubscriptions is that it is not performing any actions--it is simply creating subscriptions and we do not want to setup the same subscription call twice--that simply means the subscription handler will run twice every time the data is emitted.

.../pages/+compliance/+node-credentials/node-credentials-list/node-credential-list.component.ts

samshinde · 2020-09-24T11:35:08Z

@msorens regarding the Conflict error test case for now I have disabled the test case and looking for a solution for the same.

msorens · 2020-09-29T00:40:54Z

Re-reviewed once more; just the one item that needs addressing (#4323 (comment)), then looks good to go!

Signed-off-by: samshinde <ashinde@chef.io>

samshinde · 2020-10-07T05:03:35Z

Re-reviewed once more; just the one item that needs addressing (#4323 (comment)), then looks good to go!

@msorens Yes, I have updated the code and resolve the two API call issue.

samshinde added ui automate-ui labels Sep 9, 2020

samshinde self-assigned this Sep 9, 2020

samshinde marked this pull request as draft September 9, 2020 08:57

samshinde changed the title ~~Added consistency improvement in node credentials.~~ Added consistency improvement in list, details and create page of node credentials. Sep 10, 2020

samshinde marked this pull request as ready for review September 10, 2020 13:08

susanev requested review from a team and msorens September 15, 2020 15:51

susanev reviewed Sep 15, 2020

View reviewed changes

...e/+node-credentials/create-node-credential-modal/create-node-credential-modal.component.html Outdated Show resolved Hide resolved

susanev reviewed Sep 15, 2020

View reviewed changes

components/automate-ui/src/app/entities/node-credentials/node-credential.effects.ts Outdated Show resolved Hide resolved

susanev reviewed Sep 15, 2020

View reviewed changes

...e/+node-credentials/create-node-credential-modal/create-node-credential-modal.component.html Outdated Show resolved Hide resolved

...e/+node-credentials/create-node-credential-modal/create-node-credential-modal.component.html Outdated Show resolved Hide resolved

susanev reviewed Sep 15, 2020

View reviewed changes

samshinde force-pushed the Amol/nodeCredential_UI_Changes branch from e7db8db to 4792472 Compare September 16, 2020 07:56

msorens reviewed Sep 16, 2020

View reviewed changes

samshinde force-pushed the Amol/nodeCredential_UI_Changes branch from 4792472 to 736e78b Compare September 16, 2020 12:12

msorens reviewed Sep 18, 2020

View reviewed changes

samshinde force-pushed the Amol/nodeCredential_UI_Changes branch 2 times, most recently from b7e1d0b to 7777288 Compare September 22, 2020 09:32

susanev requested review from msorens and susanev September 23, 2020 23:15

msorens reviewed Sep 24, 2020

View reviewed changes

samshinde force-pushed the Amol/nodeCredential_UI_Changes branch from 7777288 to bda2113 Compare September 24, 2020 11:25

samshinde requested a review from msorens September 28, 2020 05:49

susanev approved these changes Sep 30, 2020

View reviewed changes

Added consistency improvement in node credentials.

aeb328e

Signed-off-by: samshinde <ashinde@chef.io>

samshinde force-pushed the Amol/nodeCredential_UI_Changes branch from bda2113 to aeb328e Compare October 5, 2020 10:56

msorens approved these changes Oct 9, 2020

View reviewed changes

msorens merged commit 0a8fe26 into master Oct 9, 2020

msorens deleted the Amol/nodeCredential_UI_Changes branch October 9, 2020 01:17

mjingle mentioned this pull request Dec 4, 2020

Update Node Credential doc screenshots #4462

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added consistency improvement in list, details and create page of node credentials. #4323

Added consistency improvement in list, details and create page of node credentials. #4323

samshinde commented Sep 9, 2020 •

edited

Loading

netlify bot commented Sep 9, 2020 •

edited

Loading

susanev commented Sep 15, 2020

samshinde commented Sep 16, 2020 •

edited

Loading

msorens left a comment

msorens left a comment

msorens left a comment

msorens Sep 24, 2020

samshinde Sep 24, 2020

msorens Sep 29, 2020

samshinde commented Sep 24, 2020

msorens commented Sep 29, 2020

samshinde commented Oct 7, 2020

Added consistency improvement in list, details and create page of node credentials. #4323

Added consistency improvement in list, details and create page of node credentials. #4323

Conversation

samshinde commented Sep 9, 2020 • 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 Sep 9, 2020 • edited Loading

susanev commented Sep 15, 2020

samshinde commented Sep 16, 2020 • edited Loading

msorens left a comment

Choose a reason for hiding this comment

msorens left a comment

Choose a reason for hiding this comment

msorens left a comment

Choose a reason for hiding this comment

msorens Sep 24, 2020

Choose a reason for hiding this comment

samshinde Sep 24, 2020

Choose a reason for hiding this comment

msorens Sep 29, 2020

Choose a reason for hiding this comment

samshinde commented Sep 24, 2020

msorens commented Sep 29, 2020

samshinde commented Oct 7, 2020

samshinde commented Sep 9, 2020 •

edited

Loading

netlify bot commented Sep 9, 2020 •

edited

Loading

samshinde commented Sep 16, 2020 •

edited

Loading