Deleting chef server message modal window #3478

sachin-msys · 2020-04-23T16:19:43Z

🔩 Description: What code changed, and why?

User should NOT be allowed to delete a chef server if it has orgs, so added message window for the same.

⛓️ Related Resources

#3294

👍 Definition of Done

Added message window on delete a chef server if it has and organisation attached to it

👟 How to Build and Test the Change

git fetch origin Sachin/infra-cookbook-details
build components/automate-ui
Go to Chef Servers option which is under Infrastructure tab and its under Chef-Server feature flag.
Add chef server and one organisation under it.
Add one more just chef server with no organisation.
Now try to delete both chef server one by one you will able to see the delete and message window as shown in attached screen shots

✅ Checklist

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

📷 Screenshots, if applicable

scottopherson · 2020-04-27T21:47:44Z

...s/automate-ui/src/app/modules/infra-proxy/chef-servers-list/chef-servers-list.component.html

+      <app-message-modal [title]="'Could Not Delete Server'" [visible]="messageModalVisible"
+        (close)="closeMessageModal()">
+        Before you can delete this server, delete all orgnisations attached to this server.
+      </app-message-modal>
      <div class="page-body">


@chef/ui-team is there consensus on listing many attributes/prop bindings on their own line or only wrapping them at a certain line length?

<app-create-chef-server-modal | [visible]="createModalVisible" | [creating]="creatingChefServer" | (close)="closeCreateModal()" | ...etc...> | </app-create-chef-server-modal> |

vs

<app-create-chef-server-modal [visible]="createModalVisible" [creating]="creatingChefServer" | (close)="closeCreateModal()" ...etc...> | </app-create-chef-server-modal> |

I've usually preferred to keep longer lists of attributes/prop bindings on their own line but don't mind either really. By default vscode wraps them by line length whenever you copy/paste html blocks

I used to be indifferent, but lately I am leaning towards one-per-line; it greatly improves readability.

changed as one-per-line 👍

@msorens yea same. Better readability in file and in git diffs. Thx @sachin-msys 👍

Signed-off-by: sachin-msys <[email protected]>

tarablack01

Nice and simple 👍🏻

msorens

Looking good; just a few corrections noted then OK to merge.

msorens · 2020-04-28T01:56:37Z

...s/automate-ui/src/app/modules/infra-proxy/chef-servers-list/chef-servers-list.component.html

+        [title]="'Could Not Delete Server'" 
+        [visible]="messageModalVisible"
+        (close)="closeMessageModal()">
+        Before you can delete this server, delete all orgnisations attached to this server.


Suggested change

Before you can delete this server, delete all orgnisations attached to this server.

Before you can delete this server, delete all organizations attached to it.

msorens · 2020-04-28T01:58:35Z

...utomate-ui/src/app/modules/infra-proxy/chef-servers-list/chef-servers-list.component.spec.ts

+      component.startServerDelete(mockChefKeyEvent, genServer('uuid-111', 0));
+       expect(component.deleteModalVisible).toBe(true);
+       component.deleteServer();
+       expect(component.deleteModalVisible).toBe(false);


Correct indents.

msorens · 2020-04-28T01:59:00Z

...utomate-ui/src/app/modules/infra-proxy/chef-servers-list/chef-servers-list.component.spec.ts

+    const mockChefKeyEvent = new KeyboardEvent('keypress') as ChefKeyboardEvent;
+    mockChefKeyEvent.isUserInput = true;
+
+    it('upon selecting delete from control menu, opens with org count 0', () => {


My first impression was the test and body do not match. But after reviewing all the code I realized that it is a language ambiguity. 😁
Please change:

Suggested change

it('upon selecting delete from control menu, opens with org count 0', () => {

it('With no orgs, selecting delete from control menu opens the delete modal', () => {

msorens · 2020-04-28T01:59:18Z

...utomate-ui/src/app/modules/infra-proxy/chef-servers-list/chef-servers-list.component.spec.ts

+    mockChefKeyEvent.isUserInput = true;
+
+
+    it('upon selecting delete from control menu, opens with org count 1', () => {


Suggested change

it('upon selecting delete from control menu, opens with org count 1', () => {

it('With one org, selecting delete from control menu opens the message modal', () => {

Also, please add one more test:
'With multiple orgs, selecting delete from control menu opens the message modal'

msorens · 2020-04-28T02:13:41Z

...utomate-ui/src/app/modules/infra-proxy/chef-servers-list/chef-servers-list.component.spec.ts

@@ -208,5 +213,56 @@ describe('ChefServersListComponent', () => {

    });
  });
+
+  describe('delete modal', () => {
+    const mockChefKeyEvent = new KeyboardEvent('keypress') as ChefKeyboardEvent;


I recently noticed this is the wrong usage and have started a PR (#3518) that will be fixing those that already exist. The problem is we should be using MatOptionSelectionChange rather than ChefKeyboardEvent. You can see the errors when we tighten up linting by changing strictOutputEventTypes to true in tsconfig.json, then running ng build.
So please change to this here and if you use it elsewhere:

Suggested change

const mockChefKeyEvent = new KeyboardEvent('keypress') as ChefKeyboardEvent;

const mockEvent = { isUserInput: true } as MatOptionSelectionChange;

Signed-off-by: sachin-msys <[email protected]>

sachin-msys · 2020-04-28T15:27:02Z

@msorens done with all suggestions please have look.

sachin-msys added ui automate-ui automate-infra-proxy labels Apr 23, 2020

sachin-msys self-assigned this Apr 23, 2020

sachin-msys linked an issue Apr 23, 2020 that may be closed by this pull request

Problem deleting chef server with attached orgs #3294

Closed

sachin-msys requested a review from a team April 27, 2020 21:24

scottopherson reviewed Apr 27, 2020

View reviewed changes

sachin-msys added 3 commits April 28, 2020 04:09

added message modal window on delete server if org is attached to it

e7d63e3

Signed-off-by: sachin-msys <[email protected]>

added some specs for message and delete modal window

998ad9a

Signed-off-by: sachin-msys <[email protected]>

code indetation corrected

f58ef18

Signed-off-by: sachin-msys <[email protected]>

sachin-msys force-pushed the Sachin/chef-server-deleting-confirmation-modal branch from 79b1c8b to f58ef18 Compare April 27, 2020 22:40

tarablack01 approved these changes Apr 27, 2020

View reviewed changes

msorens approved these changes Apr 28, 2020

View reviewed changes

corrected some typos and added some specs

bae354b

Signed-off-by: sachin-msys <[email protected]>

susanev merged commit 3040814 into master Apr 29, 2020

chef-expeditor bot deleted the Sachin/chef-server-deleting-confirmation-modal branch April 29, 2020 15:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deleting chef server message modal window #3478

Deleting chef server message modal window #3478

sachin-msys commented Apr 23, 2020

scottopherson Apr 27, 2020 •

edited

Loading

msorens Apr 27, 2020

sachin-msys Apr 27, 2020

scottopherson Apr 28, 2020

tarablack01 left a comment

msorens left a comment

msorens Apr 28, 2020

msorens Apr 28, 2020

msorens Apr 28, 2020

msorens Apr 28, 2020

msorens Apr 28, 2020 •

edited

Loading

sachin-msys commented Apr 28, 2020

	Before you can delete this server, delete all orgnisations attached to this server.
	Before you can delete this server, delete all organizations attached to it.

	it('upon selecting delete from control menu, opens with org count 0', () => {
	it('With no orgs, selecting delete from control menu opens the delete modal', () => {

		mockChefKeyEvent.isUserInput = true;


		it('upon selecting delete from control menu, opens with org count 1', () => {

	it('upon selecting delete from control menu, opens with org count 1', () => {
	it('With one org, selecting delete from control menu opens the message modal', () => {

	const mockChefKeyEvent = new KeyboardEvent('keypress') as ChefKeyboardEvent;
	const mockEvent = { isUserInput: true } as MatOptionSelectionChange;

Deleting chef server message modal window #3478

Deleting chef server message modal window #3478

Conversation

sachin-msys commented Apr 23, 2020

🔩 Description: What code changed, and why?

⛓️ Related Resources

👍 Definition of Done

👟 How to Build and Test the Change

✅ Checklist

📷 Screenshots, if applicable

scottopherson Apr 27, 2020 • edited Loading

Choose a reason for hiding this comment

msorens Apr 27, 2020

Choose a reason for hiding this comment

sachin-msys Apr 27, 2020

Choose a reason for hiding this comment

scottopherson Apr 28, 2020

Choose a reason for hiding this comment

tarablack01 left a comment

Choose a reason for hiding this comment

msorens left a comment

Choose a reason for hiding this comment

msorens Apr 28, 2020

Choose a reason for hiding this comment

msorens Apr 28, 2020

Choose a reason for hiding this comment

msorens Apr 28, 2020

Choose a reason for hiding this comment

msorens Apr 28, 2020

Choose a reason for hiding this comment

msorens Apr 28, 2020 • edited Loading

Choose a reason for hiding this comment

sachin-msys commented Apr 28, 2020

scottopherson Apr 27, 2020 •

edited

Loading

msorens Apr 28, 2020 •

edited

Loading