Added Create Environment UI #4803

himanshi-chhabra · 2021-03-05T19:46:31Z

🔩 Description: What code changed, and why?

We need to add UI for Create Environment

⛓️ Related Resources

👍 Definition of Done

Added create functionality for Environment in which we can create a particular environment.

👟 How to Build and Test the Change

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 

npm run serve:hab

To add data https://github.com/chef/automate/blob/master/dev-docs/adding-data/adding_test_data.md#adding-data-to-infra-views
PFA video for these changes.

Steps:

Go To Infrastructure -> Chef Servers -> click server name-> click on org name -> Environment
To see the create button type FEAT on-screen and a pop-up will appear you need to toggle the Infra server(Remaining) to ON and then refresh the page, will see a create button on the right of the environment tab page.

✅ 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

CreateEnvironment.mp4

netlify · 2021-03-05T19:46:35Z

Deploy preview for chef-automate ready!

Built with commit 9b9dc15

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

vinay033

Changes look good to me. Please change the screenshot.

msorens

Also, please address the semgrep error reported by Buildkite.

components/automate-ui/src/app/entities/environments/environment.effects.ts

components/automate-ui/src/app/entities/environments/environment.model.ts

...src/app/modules/infra-proxy/create-environment-modal/create-environment-modal.component.html

.../app/modules/infra-proxy/create-environment-modal/create-environment-modal.component.spec.ts

...i/src/app/modules/infra-proxy/create-environment-modal/create-environment-modal.component.ts

...src/app/modules/infra-proxy/create-environment-modal/create-environment-modal.component.html

...src/app/modules/infra-proxy/create-environment-modal/create-environment-modal.component.scss

msorens · 2021-03-13T03:12:38Z

.../app/modules/infra-proxy/create-environment-modal/create-environment-modal.component.spec.ts

+
+  describe('create environment', () => {
+    let store: Store<NgrxStateAtom>;
+    const environment = {


Can we strongly type this constant? That provides a guarantee that all the properties are valid.

Not sure why you marked this resolved; it still has no type.

yes, i have updated the changes

.../app/modules/infra-proxy/create-environment-modal/create-environment-modal.component.spec.ts

msorens · 2021-03-13T03:47:06Z

...p/modules/infra-proxy/infra-environment-constraint/infra-environment-constraint.component.ts

+      version: ['', [Validators.required, Validators.pattern(Regex.patterns.VALID_VERSION)]]
+    });
+
+    this.operatorKeys = ['~>', '>=', '>', '=', '<', '<='];


Add an in-code comment explaining ~>

added code comments.

Again, you did not read my request.
(1) Please delete the comment // operators for Cookbook version; that is self-evident.
(2) Please add an in-code comment explaining ~>

Added in-code comment for ~> : approximately greater than; also known as "pessimistically greater than", or "pessimistic"

Are you looking for this above change?

...p/modules/infra-proxy/infra-environment-constraint/infra-environment-constraint.component.ts

msorens · 2021-03-13T03:52:54Z

...p/modules/infra-proxy/infra-environment-constraint/infra-environment-constraint.component.ts

+    );
+  }
+
+  selected(value, i) {


Wow! What is this method doing?
Suggest each block should be extracted into a named function so this function will look something like:

selectLanguage(...) { setLanguageValueFromIndex(...); setNameIn???ArraytoValue(...); doSomethingSomethingWithConstraints(...); etc... }

And looking further, you can then reuse those sub-functions below in the handle* methods.

working on these changes, will update

Those handle methods are specific to name, operator, version that's why i created separated functions to handle these changes.

Looks like all you did was rename selected to handleName, etc., which is not at all what I was asking. Inside handleEditName you have what looks like 4 blocks of code. Each of those 4 blocks should be extracted into a named function so handleEditName will look something like I showed above. And then you can reuse those subfunctions in handleEditOperator and handleEditVersion.

I have added the functions for each four blocks.

...p/modules/infra-proxy/infra-environment-constraint/infra-environment-constraint.component.ts

SEAjamieD

Similar to my comments on #4813 - in the SCSS files there is a lot of fixed widths and heights being used, which is ultimately causing certain elements to remain stiff in the view. These views should all be made pretty flexible.

If you'd like more specific guidance on this, I'm happy to go more in depth, or review in a pair session.

msorens · 2021-03-16T04:49:10Z

Nice catch, @SEAjamieD !

himanshi-chhabra · 2021-03-17T12:38:19Z

Similar to my comments on #4813 - in the SCSS files there is a lot of fixed widths and heights being used, which is ultimately causing certain elements to remain stiff in the view. These views should all be made pretty flexible.

If you'd like more specific guidance on this, I'm happy to go more in depth, or review in a pair session.

@SEAjamieD Yes, I would like to have more guidance on this.

himanshi-chhabra · 2021-03-19T12:25:51Z

Similar to my comments on #4813 - in the SCSS files there is a lot of fixed widths and heights being used, which is ultimately causing certain elements to remain stiff in the view. These views should all be made pretty flexible.

If you'd like more specific guidance on this, I'm happy to go more in depth, or review in a pair session.

@SEAjamieD Hi Jamie, regarding this I have added the fixed-width of the modal in create Environment as we discussed and also added the visual indicator of which button is in focus.

...i/src/app/modules/infra-proxy/create-environment-modal/create-environment-modal.component.ts

...p/modules/infra-proxy/infra-environment-constraint/infra-environment-constraint.component.ts

msorens · 2021-03-22T04:22:31Z

...p/modules/infra-proxy/infra-environment-constraint/infra-environment-constraint.component.ts

+      version: ['', [Validators.required, Validators.pattern(Regex.patterns.VALID_VERSION)]]
+    });
+
+    this.operatorKeys = ['~>', '>=', '>', '=', '<', '<='];


Again, you did not read my request.
(1) Please delete the comment // operators for Cookbook version; that is self-evident.
(2) Please add an in-code comment explaining ~>

...p/modules/infra-proxy/infra-environment-constraint/infra-environment-constraint.component.ts

msorens · 2021-03-22T04:34:43Z

.../automate-ui/src/app/modules/infra-proxy/infra-tab-change/infra-tab-change.component.spec.ts

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


Please add a couple real tests, too.

I have removed this file as it will not specify any to run test case as I already added a test case regarding tabs in Create spec file.

msorens · 2021-03-22T04:38:06Z

...ts/automate-ui/src/app/modules/infra-proxy/infra-tab-change/infra-tab/infra-tab.component.ts

+import { Component, Input } from '@angular/core';
+
+@Component({
+  selector: 'app-infra-tab',
+  template: `
+  <div [hidden]="!active" class="test">
+    <ng-content></ng-content>
+  </div>
+`
+})
+// TODO:eng-ex The active tab should be tracked in the parent and not the individual tab
+export class InfraTabComponent {
+  @Input() tabTitle: string;
+  @Input() active = false;
+  @Input() disabled = true;
+}


Instead of making an almost exact duplicate of an existing component, please reuse the existing one (TabComponent) -- which means add the disabled property that you seem to need.

Yes, I agree with you I have already tried to use the TabComponent file but it's using class="pane" in HTML and if I try to override the CSS of that class it will override at everyplace. that's the reason I have added this file.

@SEAjamieD Could you take a look at this. Should be able to eliminate the duplication. (OK to do this in a follow-up PR.)

Yeah we definitely shouldn't be making new components that are this similar. Worst case lets extend the component we already have.

I've created a new issue to manage this situation here: #4866

msorens · 2021-03-22T04:41:27Z

Please also (a) get your unit tests passing in buildkite, and (b) talk to the releng team about getting your PR unblocked in the verify-private pipeline--that seems to be happening every time and you need to get it fixed.

himanshi-chhabra · 2021-03-22T16:46:59Z

Please also (a) get your unit tests passing in buildkite, and (b) talk to the releng team about getting your PR unblocked in the verify-private pipeline--that seems to be happening every time and you need to get it fixed.

@msorens yes, it get resolved

SEAjamieD · 2021-03-22T18:11:43Z

The biggest concern in this PR right now is that none of the buttons above are keyboard accessible. We should always be making sure that anything our users can do with a mouse, can also be done via keyboard.

SEAjamieD · 2021-03-22T18:13:41Z

Similar to my comments on #4813 - in the SCSS files there is a lot of fixed widths and heights being used, which is ultimately causing certain elements to remain stiff in the view. These views should all be made pretty flexible.
If you'd like more specific guidance on this, I'm happy to go more in depth, or review in a pair session.

@SEAjamieD Hi Jamie, regarding this I have added the fixed-width of the modal in create Environment as we discussed and also added the visual indicator of which button is in focus.

This will work for now. We should pair program sometime in the near future and I can be more explicit about what flexible means and how we should strive for flexibility. I will sync up with UX on the new designs and we can come up with a look for the Modals at small and large screen sizes, and then we can drop the new styles into all of the newly designed modals.

himanshi-chhabra · 2021-03-22T19:41:05Z

The biggest concern in this PR right now is that none of the buttons above are keyboard accessible. We should always be making sure that anything our users can do with a mouse, can also be done via keyboard.

@SEAjamieD i have added the changes

SEAjamieD · 2021-03-22T20:07:12Z

I've approved the UI changes, however I think you should wait for @msorens approval before merging

himanshi-chhabra · 2021-03-22T20:08:04Z

Similar to my comments on #4813 - in the SCSS files there is a lot of fixed widths and heights being used, which is ultimately causing certain elements to remain stiff in the view. These views should all be made pretty flexible.
If you'd like more specific guidance on this, I'm happy to go more in depth, or review in a pair session.

@SEAjamieD Hi Jamie, regarding this I have added the fixed-width of the modal in create Environment as we discussed and also added the visual indicator of which button is in focus.

This will work for now. We should pair program sometime in the near future and I can be more explicit about what flexible means and how we should strive for flexibility. I will sync up with UX on the new designs and we can come up with a look for the Modals at small and large screen sizes, and then we can drop the new styles into all of the newly designed modals.

@SEAjamieD Sure, will create a new issue regarding this.

msorens · 2021-03-23T02:16:27Z

...p/modules/infra-proxy/infra-environment-constraint/infra-environment-constraint.component.ts

+    this.operatorKeys = ['~>', // approximately greater than;
+    // also known as "pessimistically greater than", or "pessimistic"


I realize you copied this from the docs, but it is still a mystery.
Your job is to explain your code to the reader. What does it mean? This comment is inadequate.

Also, would suggest to rename the variable to operators instead of operatorKeys, to be less confusing.

msorens · 2021-03-23T02:20:22Z

...ts/automate-ui/src/app/modules/infra-proxy/infra-tab-change/infra-tab/infra-tab.component.ts

+import { Component, Input } from '@angular/core';
+
+@Component({
+  selector: 'app-infra-tab',
+  template: `
+  <div [hidden]="!active" class="test">
+    <ng-content></ng-content>
+  </div>
+`
+})
+// TODO:eng-ex The active tab should be tracked in the parent and not the individual tab
+export class InfraTabComponent {
+  @Input() tabTitle: string;
+  @Input() active = false;
+  @Input() disabled = true;
+}


@SEAjamieD Could you take a look at this. Should be able to eliminate the duplication. (OK to do this in a follow-up PR.)

msorens

Looking much better. I would like to urge that this PR be examined for missing unit tests; I point out one file below but there may be others. I would normally use "request changes" because of the lack of tests, but because of deadlines, I am approving on the condition that you will add some unit tests before you merge. 😁

msorens · 2021-03-23T02:30:46Z

...p/modules/infra-proxy/infra-environment-constraint/infra-environment-constraint.component.ts

+    this.selectedCookbookNames.forEach((element, index) => {
+      if (index === cookbookIndex) {
+        previousName = element;
+        if (newName && newName !== 'undefined') {


Is this really looking for the string "undefined" ?? Or are you trying to check if newName is undefined? If the latter, that will not work. The non-value undefined is falsy, so all you need is this:

Suggested change

if (newName && newName !== 'undefined') {

if (newName) {

Note that if this was unit-tested, I would not have to ask the question. 😁
And I am just noticing that this file does not have any unit tests; there is a lot going on here and it definitely needs tests.

Signed-off-by: Himanshi Chhabra <himanshi.chhabra@msystechnologies.com>

Signed-off-by: vinay sharma <vsharma@chef.io>

Signed-off-by: Himanshi Chhabra <himanshi.chhabra@msystechnologies.com>

himanshi-chhabra · 2021-03-25T14:11:14Z

While acceptance testing, observed

In Create Environment, disable create button if a name already exists.
change the border color of the text-field when focused.
will cover in UI Improvements for Edit View #4873

himanshi-chhabra added ui automate-ui automate-infra-proxy labels Mar 5, 2021

himanshi-chhabra self-assigned this Mar 5, 2021

vinay033 self-requested a review March 5, 2021 20:19

vinay033 approved these changes Mar 5, 2021

View reviewed changes

msorens suggested changes Mar 11, 2021

View reviewed changes

himanshi-chhabra mentioned this pull request Mar 11, 2021

UI Improvements according to UX review in Edit Infra View #4808

Closed

himanshi-chhabra force-pushed the Himanshi/create_Environment_UI branch from fd506a0 to 3a7e6bb Compare March 11, 2021 16:28

msorens suggested changes Mar 13, 2021

View reviewed changes

SEAjamieD suggested changes Mar 15, 2021

View reviewed changes

himanshi-chhabra force-pushed the Himanshi/create_Environment_UI branch from f87ccb0 to b7eebcb Compare March 19, 2021 12:20

msorens reviewed Mar 22, 2021

View reviewed changes

himanshi-chhabra force-pushed the Himanshi/create_Environment_UI branch from b7eebcb to c620ee8 Compare March 22, 2021 11:31

himanshi-chhabra force-pushed the Himanshi/create_Environment_UI branch from a9267da to 41ffd65 Compare March 22, 2021 19:39

SEAjamieD approved these changes Mar 22, 2021

View reviewed changes

msorens reviewed Mar 23, 2021

View reviewed changes

msorens approved these changes Mar 23, 2021

View reviewed changes

Added UI for create environment.

3d60dee

Signed-off-by: Himanshi Chhabra <himanshi.chhabra@msystechnologies.com>

vinay033 and others added 16 commits March 23, 2021 15:42

fixed CSS for the create env tabs

ec8ff46

Signed-off-by: vinay sharma <vsharma@chef.io>

Added some css changes.

795a008

Signed-off-by: Himanshi Chhabra <himanshi.chhabra@msystechnologies.com>

Added testcases for create.

bc8ad08

Signed-off-by: Himanshi Chhabra <himanshi.chhabra@msystechnologies.com>

Added reset constraint changes.

c393cba

Signed-off-by: Himanshi Chhabra <himanshi.chhabra@msystechnologies.com>

Added some css updates and comments.

cb65252

Signed-off-by: Himanshi Chhabra <himanshi.chhabra@msystechnologies.com>

Added css variable changes.

bf79e99

Signed-off-by: Himanshi Chhabra <himanshi.chhabra@msystechnologies.com>

Added some lint error changes.

db36836

Signed-off-by: Himanshi Chhabra <himanshi.chhabra@msystechnologies.com>

Added UX improvements changes and implemented review comments.

7e2e349

Signed-off-by: Himanshi Chhabra <himanshi.chhabra@msystechnologies.com>

Implemented review comments.

0273c67

Signed-off-by: Himanshi Chhabra <himanshi.chhabra@msystechnologies.com>

Resolved unit test case error.

eb2dcc6

Signed-off-by: Himanshi Chhabra <himanshi.chhabra@msystechnologies.com>

Added fixed height and tab button focus changes.

8c32cb0

Signed-off-by: Himanshi Chhabra <himanshi.chhabra@msystechnologies.com>

Added review comments

df41960

Signed-off-by: Himanshi Chhabra <himanshi.chhabra@msystechnologies.com>

Added pipeline issue changes.

9599df6

Signed-off-by: Himanshi Chhabra <himanshi.chhabra@msystechnologies.com>

Added keyboard tab functionality changes on anchor tab.

a56616c

Signed-off-by: Himanshi Chhabra <himanshi.chhabra@msystechnologies.com>

Added review comments changes.

3e0170d

Signed-off-by: Himanshi Chhabra <himanshi.chhabra@msystechnologies.com>

Added testcase and comment line changes.

9b9dc15

Signed-off-by: Himanshi Chhabra <himanshi.chhabra@msystechnologies.com>

himanshi-chhabra force-pushed the Himanshi/create_Environment_UI branch from da197b7 to 9b9dc15 Compare March 23, 2021 10:13

kalroy merged commit 20d2f27 into master Mar 23, 2021

kalroy deleted the Himanshi/create_Environment_UI branch March 23, 2021 11:26

himanshi-chhabra added the acceptance: issue label Mar 25, 2021

		this.operatorKeys = ['~>', // approximately greater than;
		// also known as "pessimistically greater than", or "pessimistic"

Added Create Environment UI #4803

Added Create Environment UI #4803

Conversation

himanshi-chhabra commented Mar 5, 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 5, 2021 • edited Loading

vinay033 left a 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

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 left a comment

Choose a reason for hiding this comment

msorens commented Mar 16, 2021

himanshi-chhabra commented Mar 17, 2021

himanshi-chhabra commented Mar 19, 2021 • edited Loading

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 commented Mar 22, 2021

himanshi-chhabra commented Mar 22, 2021

SEAjamieD commented Mar 22, 2021

SEAjamieD commented Mar 22, 2021

himanshi-chhabra commented Mar 22, 2021

SEAjamieD commented Mar 22, 2021

himanshi-chhabra commented Mar 22, 2021

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

himanshi-chhabra commented Mar 25, 2021

himanshi-chhabra commented Mar 5, 2021 •

edited

Loading

netlify bot commented Mar 5, 2021 •

edited

Loading

himanshi-chhabra commented Mar 19, 2021 •

edited

Loading