Infra UI cleanup -- and learning resource! #4875

SEAjamieD · 2021-03-25T19:02:14Z

🔩 Description: What code changed, and why?

This was a rough and quick first pass at correcting code throughout the infrastructure pages where we've noticed a multitude of mistakes begin to present themselves. While this doesn't cover every piece that needs to be addressed, it shows the depth of the work needed and can hopefully be used as a learning resource as well--both for code authors and for code reviewers!

As you go through this, keep PR #4874 open next to it, so you can see how that revised checklist helps to point you towards things covered here.

⛓️ Related Resources

msorens' remarks: Following on Jamie's introduction above, I thought this would be a good place to reiterate guidelines I put together (over a span of years!) on authoring and on reviewing code.

Reviewing code is just as important as writing code. Really. Here are some important reasons why:

It promotes knowledge transfer.
It shakes out different ideas/strategies.
It encourages team consistency and communication.
It helps you improve your craft.
It reduces buggy software very cost effectively--as close to inception as possible.

As a Code Author

Be your own first code reviewer. Go through your code thoroughly and provide pre-review comments to your audience. Also look for things your IDE did to "help" you (reformatting, etc).
Distinguish between in-code comments, which describe what is present, and pre-review comments, which describe why it changed.
Also use pre-review comments to filter out noise (e.g. "only function xyz changed in this file; the other 95% of the changes were just reformatting").
Include only one issue in a code review (i.e. apply Single Responsibility Principle) for focus, traceability, and tidiness. It also makes searching commit histories more straightforward.
Exception to above: for those really small or simple–but unrelated–things that are just simpler to include now rather than later, mark them as such by beginning with something like "PR Bonus: ...".
Include all of one issue including, but not limited to: user interface changes, application code changes, documentation updates, help text revisions, database schema revisions, unit test additions, configuration file updates, build script changes, installer tweaks... and do not forget the new files you added! (Note that this is not advocating an entire, humongous feature at one go; rather whatever size piece you are working on, make it complete.)
Rebase your changes so they are on top of the latest code from the main branch before sending out your review.
Verify the four cornerstones: unit tests, lint, build, execution.
Make your presentation shine: spell check, no leftover TODO's, no locally tweaked configuration files, no orphaned properties/variables/files, no missing files.
Always make changes for a reason: you should be able to justify every character you are changing.

As a Code Reviewer

Ultimately your task is to answer the question: do the changes accurately and completely cover the requirements?

Understand the requirements of the issue before you dig into the code, including reading the issue and other pertinent background material.
If there are UI changes, was a screenshot included?
Compare requirements against unit tests (every requirement should be tested)
Compare unit tests against the code under test (all code should be tested)
Review the unit tests as an ensemble (to look for weaknesses, ambiguities, or gaps)
Is each unit test named well? (unambiguous, accurate, complete)
Does each unit test evaluate only one thing?
Do the unit tests have strong coverage? (equivalence class and boundary value analysis)
Is each unit test as simple as possible? (ideally cyclomatic complexity === 1)
Does the code under test follow best practices? (SOLID principles, no magic numbers, etc.)
Can you get by with less code? (is there a simpler way?)
Look beyond the changes, from the low-level (if FooBar is changed to FooBaz, are there instances of Foo-Bar as well?) to the high-level (was the README updated correspondingly?)
Is the code self-documenting? (that is the code itself, minimizing comments)

References

My Zen of Code Review series:

Part 1: Pre-Review Comments (https://bit.ly/3imYCoO)
Part 2: Authoring Best Practices (https://bit.ly/35PJVpi)
Part 3: The Reviewer's Take (https://bit.ly/2XQQTpS)
Part 4: Review as if you Own the Code (https://bit.ly/3qsNVnv)
The Sequel: Better Code Reviews with GIT (https://bit.ly/38SveDW)

👍 Definition of Done

For this particular PR "Done" means we've effectively begun a cleanup as well as communicated the need for further review.

👟 How to Build and Test the Change

in hab studio
build components/automate-ui-devproxy && start_all_services

in Automate/components/automate-ui folder
make serve

✅ 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

While many of the fixes are code-based only, much has been showing up in the UI as added white spaces, missing focus rings, 5 to 10 pixel off alignments, etc.
Below is one example

Before
Search button sits outside the Search container with extra white space at the bottom. Create environment button sits below the place of the search bar.

After

After
Accessibility rings for the input and search button, Text visible even when screen very condensed.

Signed-off-by: michael sorens <[email protected]>

Best way to see the consistency/inconsistency is to search for all occurrences of "message:" in *.effects.ts. Signed-off-by: michael sorens <[email protected]>

Signed-off-by: michael sorens <[email protected]>

Signed-off-by: seajamied <[email protected]>

…ough These should really get worked through in a more global sense. I'll connect with UX about how these will look in the future Signed-off-by: seajamied <[email protected]>

Signed-off-by: seajamied <[email protected]>

I mostly reworked the search bar first so that it is accessible and next to get all the pixels in the right place. It could likely be cleaned up more as well but I think this is a great start Signed-off-by: seajamied <[email protected]>

Signed-off-by: seajamied <[email protected]>

netlify · 2021-03-25T19:02:19Z

Deploy preview for chef-automate processing.

Building with commit 27a81ec

https://app.netlify.com/sites/chef-automate/deploys/605d43daa01e330008617248

msorens

Lots of author notes below. You will see a high correlation between these types of fixes and the new suggested PR template in #4874

msorens · 2021-03-25T19:04:35Z

components/automate-ui/src/app/entities/cds/cds.effects.ts

@@ -114,7 +114,7 @@ export class CdsEffects {
    map((action: DownloadContentItemSuccess) => {
      return new CreateNotification({
        type: Type.info,
-        message: `Content Item "${action.payload.name}" was download`
+        message: `Content Item "${action.payload.name}" was downloaded`


Author note: grammar

msorens · 2021-03-25T19:05:07Z

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

@@ -119,7 +119,7 @@ export class ClientEffects {
      map(({ payload: { name } }: DeleteClientSuccess) => {
        return new CreateNotification({
          type: Type.info,
-          message: `Successfully Deleted Client - ${name}.`
+          message: `Successfully deleted client - ${name}.`


Author note: case consistency -- only the first word should be capitalized.

msorens · 2021-03-25T19:05:41Z

components/automate-ui/src/app/entities/data-bags/data-bag-details.effects.ts

@@ -51,7 +51,7 @@ export class DataBagItemsEffects {
      const msg = payload.error.error;
      return new CreateNotification({
        type: Type.error,
-        message: `Could not get infra data bag items: ${msg || payload.error}`
+        message: `Could not get data bag items: ${msg || payload.error}`


Author note: consistency -- majority of uses omit the "infra" so several occurrences changed.

msorens · 2021-03-25T19:06:37Z

components/automate-ui/src/app/entities/data-bags/data-bags.effects.ts

@@ -64,7 +64,7 @@ export class DataBagsEffects {
    map(({ payload: { databag: dataBag } }: CreateDataBagSuccess) => {
      return new CreateNotification({
        type: Type.info,
-        message: `Successfully Created Data Bag ${dataBag.name}.`
+        message: `Successfully created data bag - ${dataBag.name}.`


Author note: consistency -- this new pattern is supposed to also include the hyphen.

msorens · 2021-03-25T19:11:50Z

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

@@ -130,7 +130,7 @@ export class EnvironmentEffects {
      filter(({ payload }: CreateEnvironmentFailure) => payload.status !== HttpStatus.CONFLICT),
      map(({ payload }: CreateEnvironmentFailure) => new CreateNotification({
        type: Type.error,
-        message: `Could not create notification: ${payload.error.error || payload}.`
+        message: `Could not create environment: ${payload.error.error || payload}.`


Author note: copy/paste error

msorens · 2021-03-25T19:26:37Z

...tomate-ui/src/app/modules/infra-proxy/environment-details/environment-details.component.html

          [jsonText]="jsonText"
          [openEvent]="openEnvironmentModal"
-          [serverId]="serverId" [orgId]="orgId"
-          [name]="name" [environment]="environment"
+          [serverId]="serverId"
+          [orgId]="orgId"
+          [name]="name"
+          [environment]="environment"
          [label]="label"
          [cookbookConstraints]="cookbookConstraints"
          [cookbookVersions]="cookbookVersions">


Author note: clarity -- do not hide some attributes by putting two on a line just in a couple cases. If short enough, OK to put all on one line. Otherwise, each on a line by itself.

msorens · 2021-03-25T19:27:20Z

...tomate-ui/src/app/modules/infra-proxy/environment-details/environment-details.component.html

-                  <img alt="Edit" src="/assets/img/content and icon.png" />
+                  <img alt="Edit" src="/assets/img/content-and-icon.png" />


Author note: consistency -- no other file uses spaces in the name. (Besides, having spaces in a filename is asking for maintenance headaches.)

msorens · 2021-03-25T19:27:39Z

...tomate-ui/src/app/modules/infra-proxy/environment-details/environment-details.component.html

-            <chef-button class="add-circle" primary (click)="tre.expand()">
+            <chef-button class="add-circle" primary (click)="tree.expand()">


Author note: typo -- running make spell would have caught this.

msorens · 2021-03-25T19:28:07Z

...tomate-ui/src/app/modules/infra-proxy/environment-details/environment-details.component.scss

-  color: #000000;
+  color: $chef-black;


Author note: Do not use color literals in CSS files; always use the provided named constants.

msorens · 2021-03-25T19:29:18Z

components/automate-ui/src/app/modules/infra-proxy/select-box/src/lib/select-box.component.html

-                    [(ngModel)]="leftFilterText" [disabled]="disabled" placeholder="Search.." />
+                    [(ngModel)]="leftFilterText" [disabled]="disabled" placeholder="Search.." /> <!-- TODO: do not use two-way binding! -->


A key tenet of automate design is One-Way Binding. The [(ngModel)] uses two-way binding and needs fixing; just have not done it here.

Agreed, will fix these changes in #4540.

Signed-off-by: seajamied <[email protected]>

SEAjamieD · 2021-03-26T00:11:47Z

Another discovery I came across during this exercise that will need to be examined is the way we are choosing to filter within the infra-search-bar, as we are not catching upper vs lowercase.

Signed-off-by: michael sorens <[email protected]>

vivek-yadav · 2021-03-26T13:36:27Z

components/automate-ui/src/app/entities/infra-roles/infra-role.effects.ts

@@ -48,7 +48,7 @@ export class InfraRoleEffects {
        const msg = payload.error.error;
        return new CreateNotification({
          type: Type.error,
-          message: `Could not get infra roles: ${msg || payload.error}`
+          message: `Could not get roles: ${msg || payload.error}`


Would infra roles make more sense, as just roles might be confused with internal roles ?

Good question! I considered that, but decided against it for two reasons.

Omitting it is more consistent with the rest of the infra pages.

More importantly, the context makes it clear which type of role.
That said, if you have a strong preference I am fine to change it because your point is valid.

vivek-yadav · 2021-03-26T13:38:44Z

components/automate-ui/src/app/entities/orgs/org.effects.ts

@@ -118,7 +118,7 @@ export class OrgEffects {
      map(({ payload: { name } }: DeleteOrgSuccess) => {
        return new CreateNotification({
          type: Type.info,
-          message: `Successfully Deleted Organization ${name}.`
+          message: `Successfully deleted organization ${name}.`


is - needed before ${name}

Yes! Good eye! 🔍

vivek-yadav · 2021-03-26T13:39:20Z

components/automate-ui/src/app/entities/orgs/org.effects.ts

@@ -147,7 +147,7 @@ export class OrgEffects {
      ofType(OrgActionTypes.UPDATE_SUCCESS),
      map(({ payload: { org } }: UpdateOrgSuccess) => new CreateNotification({
      type: Type.info,
-      message: `Successfully Updated Organization ${org.name}.`
+      message: `Successfully updated organization ${org.name}.`


Is - needed before ${org.name} ?

vivek-yadav · 2021-03-26T13:39:57Z

components/automate-ui/src/app/entities/policies/policy.effects.ts

@@ -97,7 +97,7 @@ export class PolicyEffects {
      const removeStr = removed.length === 1 ? removed[0].displayName : `${removed.length} members`;
      return new CreateNotification({
          type: Type.info,
-          message: `Removed ${removeStr}.`
+          message: `Removed member(s) ${removeStr}.`


Is - needed before ${removeStr} ?

OK, the answer here is "no". This is still the old format, the difference is the word "Successfully" is not present here. Jon from UX is well aware of the inconsistencies--I spoke to him about it a couple days ago. Apparently, new code we want to go in with "Successfully - " but I purposely did not change this one because it is "old" code.

vivek-yadav · 2021-03-26T13:58:21Z

components/automate-ui/src/app/pages/+compliance/+scanner/state/scanner.effects.ts

@@ -192,7 +192,7 @@ export class ScannerEffects {
    }),
    map(() => new CreateNotification({
      type: Type.info,
-      message: 'Deleted a scan job.'
+      message: 'Deleted scan job.'


is ID of Deleted scan job needed here ? if yes, we can add a todo for later, here also.

Yes, should mark with TODO. Every such message should have an ID.

kalroy

Thanks for this detailed information.

vinay033

Greate work @SEAjamieD , Thanks for these changes. :)

himanshi-chhabra

Changes Look good to me.
Just one opinion, as per new Notification UX in success/update notifications we have added Successfully text, and in some cases, it's not added.
eg: In infra-role: L88 : message: `Created role ${name}
and in databag-item: L71: message: Successfully created data bag item ${id}.

we can make these changes in this issue: #4873

himanshi-chhabra · 2021-04-05T14:36:40Z

Another discovery I came across during this exercise that will need to be examined is the way we are choosing to filter within the infra-search-bar, as we are not catching upper vs lowercase.

@SEAjamieD According to chef-manage, we have a case-sensitive filter within infra-search-bar through API. I have shared a scenario below files.

SEAjamieD · 2021-04-05T15:53:00Z

Another discovery I came across during this exercise that will need to be examined is the way we are choosing to filter within the infra-search-bar, as we are not catching upper vs lowercase.

Thanks @himanshi-chhabra - Is this by design? or should we update this to catch both upper and lowercase?

msorens and others added 22 commits March 25, 2021 09:47

Correct form name

cf080f0

Signed-off-by: michael sorens <[email protected]>

Correct and make consistent all notification messages

030ba32

Best way to see the consistency/inconsistency is to search for all occurrences of "message:" in *.effects.ts. Signed-off-by: michael sorens <[email protected]>

Apply more useful variable names

6eb4851

Signed-off-by: michael sorens <[email protected]>

Clean up and streamline duplicated code

dd3a143

Signed-off-by: michael sorens <[email protected]>

Simplify use of backticks

1b88911

Signed-off-by: michael sorens <[email protected]>

Improve readability/consistency in HTML

cfe01af

Signed-off-by: michael sorens <[email protected]>

Add TODO for removing two-way binding

58dbbd6

Signed-off-by: michael sorens <[email protected]>

Best practice: do not use spaces in file names

6cf7ca0

Signed-off-by: michael sorens <[email protected]>

Fix typo in variable name

59eb1fe

Signed-off-by: michael sorens <[email protected]>

Remove hard-coded colors

e051f50

Signed-off-by: michael sorens <[email protected]>

Correct/cleanup use of json.stringify

0d48ca6

Signed-off-by: michael sorens <[email protected]>

Correct error text to match actual field names

247a064

Signed-off-by: michael sorens <[email protected]>

Add missing "required" indicators

34a29e8

Signed-off-by: michael sorens <[email protected]>

Add strong type

7ec0f47

Signed-off-by: michael sorens <[email protected]>

Delete self-evident comments

b64465c

Signed-off-by: michael sorens <[email protected]>

fixup! Add strong type

286c9cb

Fix white space that is popping out of side of table header

35f69c5

Signed-off-by: seajamied <[email protected]>

Small pixel updates to remove rounded corners that let white show thr…

65213ef

…ough These should really get worked through in a more global sense. I'll connect with UX about how these will look in the future Signed-off-by: seajamied <[email protected]>

WIP - lots of cleanup visually in the infra-search-bar

ea8db94

Signed-off-by: seajamied <[email protected]>

WIP - padding cleanup

a9dbb20

Signed-off-by: seajamied <[email protected]>

Reiterate on infra-search-bar

e5ec8a0

I mostly reworked the search bar first so that it is accessible and next to get all the pixels in the right place. It could likely be cleaned up more as well but I think this is a great start Signed-off-by: seajamied <[email protected]>

Add test for new focus ring

0892a7c

Signed-off-by: seajamied <[email protected]>

msorens reviewed Mar 25, 2021

View reviewed changes

seajamied added 6 commits March 25, 2021 13:09

Small cleanup

4cb344c

Signed-off-by: seajamied <[email protected]>

Eliminate unnecessary functions

1890757

Signed-off-by: seajamied <[email protected]>

Adjust naming to be more generic

bc4594e

Signed-off-by: seajamied <[email protected]>

Alignment cleanup in Environments

34c7168

Signed-off-by: seajamied <[email protected]>

Update Clients alignment

95f4cb1

Signed-off-by: seajamied <[email protected]>

Lint fixes

c5f78e2

Signed-off-by: seajamied <[email protected]>

SEAjamieD assigned SEAjamieD and msorens and unassigned SEAjamieD Mar 25, 2021

SEAjamieD added automate-ui ui labels Mar 25, 2021

SEAjamieD marked this pull request as ready for review March 25, 2021 23:52

Oops! Correct one overzealous correction.

27a81ec

Signed-off-by: michael sorens <[email protected]>

msorens force-pushed the jamie/ms-infra-cleanup-copy branch from 9fdbec6 to 27a81ec Compare March 26, 2021 02:15

msorens changed the title ~~Infra UI cleanup~~ Infra UI cleanup -- and learning resource! Mar 26, 2021

vivek-yadav reviewed Mar 26, 2021

View reviewed changes

vivek-yadav approved these changes Mar 26, 2021

View reviewed changes

SEAjamieD mentioned this pull request Mar 30, 2021

Infra proxy edit mode - added some UI improvements #4888

Merged

4 tasks

vivek-yadav requested review from vivekshankar1 and shaik80 April 1, 2021 16:20

SEAjamieD requested review from himanshi-chhabra, vinay033 and chaitali-mane April 2, 2021 22:25

kalroy approved these changes Apr 5, 2021

View reviewed changes

vinay033 approved these changes Apr 5, 2021

View reviewed changes

himanshi-chhabra approved these changes Apr 5, 2021

View reviewed changes

SEAjamieD merged commit 239f829 into master Apr 5, 2021

SEAjamieD deleted the jamie/ms-infra-cleanup-copy branch April 5, 2021 15:52

kalroy added the acceptance: internal label Apr 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Infra UI cleanup -- and learning resource! #4875

Infra UI cleanup -- and learning resource! #4875

SEAjamieD commented Mar 25, 2021 •

edited by msorens

Loading

netlify bot commented Mar 25, 2021 •

edited

Loading

msorens left a comment

msorens Mar 25, 2021

msorens Mar 25, 2021

msorens Mar 25, 2021

msorens Mar 25, 2021

msorens Mar 25, 2021

msorens Mar 25, 2021

msorens Mar 25, 2021

msorens Mar 25, 2021

msorens Mar 25, 2021

msorens Mar 25, 2021

himanshi-chhabra Apr 5, 2021

SEAjamieD commented Mar 26, 2021

vivek-yadav Mar 26, 2021

msorens Mar 26, 2021

vivek-yadav Mar 26, 2021

msorens Mar 26, 2021

vivek-yadav Mar 26, 2021

vivek-yadav Mar 26, 2021

msorens Mar 26, 2021

vivek-yadav Mar 26, 2021

msorens Mar 26, 2021

kalroy left a comment

vinay033 left a comment

himanshi-chhabra left a comment •

edited

Loading

himanshi-chhabra commented Apr 5, 2021

SEAjamieD commented Apr 5, 2021

		<img alt="Edit" src="/assets/img/content and icon.png" />
		<img alt="Edit" src="/assets/img/content-and-icon.png" />

		<chef-button class="add-circle" primary (click)="tre.expand()">
		<chef-button class="add-circle" primary (click)="tree.expand()">

		[(ngModel)]="leftFilterText" [disabled]="disabled" placeholder="Search.." />
		[(ngModel)]="leftFilterText" [disabled]="disabled" placeholder="Search.." /> <!-- TODO: do not use two-way binding! -->

Infra UI cleanup -- and learning resource! #4875

Infra UI cleanup -- and learning resource! #4875

Conversation

SEAjamieD commented Mar 25, 2021 • edited by msorens Loading

🔩 Description: What code changed, and why?

⛓️ Related Resources

As a Code Author

As a Code Reviewer

References

👍 Definition of Done

👟 How to Build and Test the Change

✅ Checklist

📷 Screenshots, if applicable

netlify bot commented Mar 25, 2021 • edited Loading

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

SEAjamieD commented Mar 26, 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

Choose a reason for hiding this comment

kalroy left a comment

Choose a reason for hiding this comment

vinay033 left a comment

Choose a reason for hiding this comment

himanshi-chhabra left a comment • edited Loading

Choose a reason for hiding this comment

himanshi-chhabra commented Apr 5, 2021

SEAjamieD commented Apr 5, 2021

SEAjamieD commented Mar 25, 2021 •

edited by msorens

Loading

netlify bot commented Mar 25, 2021 •

edited

Loading

himanshi-chhabra left a comment •

edited

Loading