Jamie/2523 data lifecycle refactor #3158

SEAjamieD · 2020-03-19T18:40:32Z

🔩 Description: What code changed, and why?

This branch updates the Node Lifecycles Page to now be named Data Lifecycles, and will allow the user more options in controlling their data retention choices through the UI

⛓️ Related Resources

Finishes: #1208
Makes use of: #2157
UX Design File: https://app.abstract.com/share/47da49f4-55df-4be0-be66-018952857ae3?mode=design&sha=84902a48f104fe6bb5f4cefa3754cc83a72ccbe0

👍 Definition of Done

User now has the ability to update all areas available through the API. Currently this includes Event Feed, Service Groups, Compliance, and will soon include Services Groups

👟 How to Build and Test the Change

build components/automate-ui-devproxy && start_all_services
make serve

navigate to https://a2-dev.test/settings/data-lifecycle
make any changes you wish to see in the form and submit - see a "success" notification at top of screen.

You should not:
Be able to enter any negative numbers into inputs
Be allowed to submit the form until there has been some sort of change made at least once

✅ 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

SEAjamieD · 2020-03-19T18:43:34Z

Please note this PR is only currently ready for UX review

SEAjamieD · 2020-03-20T01:04:11Z

This is now open for review to all - thanks!

@msorens - please note: I have not yet changed the name of the component to something more specific. This components was already in the system and a search on it linked me to ~152 files. I'd like to separate that out as a separate ticket so as not to hold this one back.

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

components/automate-ui/src/app/entities/automate-settings/automate-settings.requests.ts

SEAjamieD · 2020-03-20T01:09:13Z

components/automate-ui/src/app/entities/automate-settings/automate-settings.requests.ts

+  private unfurlIngestJob(job: IngestJob, nested: boolean = false): UnfurledJob {
+    if (nested) {
+      return {
+        policy_name: job.nested_name,
+        older_than_days: parseInt(job.threshold, 10),
+        disabled: job.disabled
+      };
+    } else {
+      return {
+        name: job.name,
+        threshold: job.threshold,
+        disabled: job.disabled
+      };
+    }
+  }
+
+


This function is taking the job, and placing in the required format based on whether or not it is nested in the API Calls

SEAjamieD · 2020-03-20T01:11:40Z

components/automate-ui/src/app/entities/automate-settings/automate-settings.requests.ts

  private convertResponseToJobSchedulerStatus(
    respJobSchedulerStatus: RespJobSchedulerStatus): JobSchedulerStatus {
-      const jobs = respJobSchedulerStatus.jobs.map((respJob: RespJob) => new IngestJob(respJob));
-      return new JobSchedulerStatus(respJobSchedulerStatus.running, jobs);
+
+    const allJobs = [];
+
+    Object.keys(respJobSchedulerStatus).forEach((category: string) => {
+      if (respJobSchedulerStatus[category]) {
+        const catJobs = respJobSchedulerStatus[category].jobs
+          .map((respJob: RespJob) => new IngestJob(category, respJob));
+        allJobs.push(...catJobs);
+      }
+    });
+
+    return new JobSchedulerStatus(allJobs);
  }


This functions purpose is to take the response we get back when requesting the configurations, and untangling it so that we can place it back in the form for the user to see. I had to start by separating them by category, infra, compliance, services, event_feed.

SEAjamieD · 2020-03-20T01:14:40Z

components/automate-ui/src/app/pages/automate-settings/automate-settings.component.spec.ts

    describe('when user applyChanges()', () => {
      it('saves settings', () => {
        component.updateForm(mockJobSchedulerStatus);
        component.applyChanges();
-        expect(component.formChanged).toEqual(false);
+
+        // expect(component.notificationVisible).toBe(true);
        expect(component.notificationType).toEqual('info');
        expect(component.notificationMessage)
-          .toEqual('All settings have been updated successfully');
-        expect(component.notificationVisible).toEqual(true);
+        .toEqual('All settings have been updated successfully');
+        expect(component.formChanged).toEqual(false);
+        // expect(component.saving).toEqual(false);
      });


this is a testing issue that I'm not yet sure how to resolve and could use some help. Visually this is working as expected and i've located the reason why the test is failing but not sure how to resolve yet

susanev

really great work!! these are a few things I noticed...

im noticing some weird behaviour with the save changes button, when first navigating to the page the button is enabled, but it should be disabled since I haven't made any changes yet.
any chance we could add a tooltip over the applications checkboxes that says something like Changing this setting in the UI is not supported. or similar?
there's 24 px of space between the nested ones, can we reduce that to 12px?
the numbered inputs get a darker gray color when they are disabled can we do the same for the dropdowns?
can you remove all the margins from that save changes button?
if there's anyway we can remove this gray background on this page, that would be rad. but not critical.

susanev · 2020-03-20T03:24:23Z

this is totally my fault, but I think notifications and data feeds belong in the same section in the left nav. I've got a question out to the belfast team to figure out if they are best to be in general settings or in node management and ill follow up with their answer tomorrow.

susanev · 2020-03-20T15:01:21Z

following up, lets move data feeds up to the general settings section, so notifications, data feeds, data lifecycle for the order

SEAjamieD · 2020-03-20T18:16:10Z

Came across some a11y issues today for keyboard navigation, adding to list of items to remedy

SEAjamieD · 2020-03-20T19:15:03Z

@susanev

✅ Tooltip over services checkboxes --> let me know if you'd like me to change anything about these

✅ there's 24 px of space between the nested ones, can we reduce that to 12px?

✅ darken disabled dropdown color --> is the disabled dropdown text lighter than input or are my eyes bad?

✅ can you remove all the margins from that save changes button?

✅ if there's anyway we can remove this gray background on this page, that would be rad. but not critical.

✅ Rearrange Sidebar

Can you give me another look on the Save Button being enabled on enter? This is working on my computer reliably and curious if its still happening.

msorens

About half-way through--looking very good!

components/automate-ui/e2e/deprecated-routes.e2e-spec.ts

msorens · 2020-03-20T19:07:39Z

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

@@ -73,7 +73,7 @@ const routes: Routes = [
          component: SettingsLandingComponent
        },
        {
-          path: 'node-lifecycle',
+          path: 'data-lifecycle',


This change in nomenclature warrants changes to node-lifecycle.md and/or data-lifecycle.md.

@kagarmoe / @mjingle - Making sure I don't step on any toes here, is this part of documentation or is this a change that I should handle?

I think this is being taken care of already? We're removing any instance of node-lifecycle in the docs per #3170
Also, I'm pretty sure that we're deleting node-lifecycle.md and creating data-lifecycle.md in its place.

data lifecycle docs page already existed, but yah deleting the node lifecycle page.. with a redirect.

ok great - thats the info I was looking for. Thank you!

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

components/automate-ui/src/app/entities/automate-settings/automate-settings.model.ts

components/automate-ui/src/app/entities/automate-settings/automate-settings.requests.ts

components/automate-ui/src/app/entities/automate-settings/automate-settings.model.ts

components/automate-ui/src/app/entities/automate-settings/automate-settings.requests.ts

susanev · 2020-03-21T01:01:40Z

@SEAjamieD heres a video of the save button stuff I mentioned

msorens

A few more observations.

components/automate-ui/src/app/pages/automate-settings/automate-settings.component.html

components/automate-ui/src/app/pages/automate-settings/automate-settings.component.scss

components/automate-ui/src/app/pages/automate-settings/automate-settings.component.spec.ts

components/automate-ui/src/app/pages/automate-settings/automate-settings.component.ts

components/automate-ui/src/app/pages/automate-settings/automate-settings.component.spec.ts

susanev · 2020-03-22T19:24:11Z

@SEAjamieD

theres an rgba(0, 0, 0, 0.38); on the dropdown when its disabled, so i think thats why the text color appears lighter than the numerical input
gray background, still appears at the very bottom of the page, we may be able to leverage whatever they did on the applications page to stop this from happening, but this is super low priority so if its not quick to fix dont worry about it

components/automate-ui/src/app/pages/automate-settings/automate-settings.component.spec.ts

components/automate-ui/src/app/pages/automate-settings/automate-settings.component.ts

components/automate-ui/src/environments/environment.ts

components/automate-ui/src/app/pages/automate-settings/automate-settings.component.html

components/automate-ui/src/app/pages/automate-settings/automate-settings.component.ts

msorens · 2020-03-27T23:53:49Z

components/automate-ui/src/app/pages/automate-settings/automate-settings.component.ts

+      if (jobForm.threshold === null) {
+        job.threshold = '0' + jobForm.unit;
+      } else {
+        job.threshold = jobForm.threshold + jobForm.unit;


Ah, finally found it. I saw splitThreshold and figured out you were packing a value and unit together so this is where that happens. Is there a reason you cannot use a structure comprised of two properties value and unit? That would obviate the need for the somewhat obfuscatory pack and split.

This is one of those cases that is needed because of the difference between the older pieces of the API and the newer pieces of the API.

The older pieces of the API store/read the threshold value as a string with both the unit and threshold values. For example, if the value is "4 days", the threshold on this portion of the api is stored as "4d".

The newer portions of the API are only storing the value for the amount of days to between retention. and they are storing them as a number. ie. numeric 4.

From how i'm looking at it, we can't really avoid packing and splitting. But if theres some other way that i'm not seeing - i'm all ears.

If possibly you're suggesting simply renaming job.threshold to something like 'job.packed_thresholdso that ultimately that line would read asjob.packed_threshold = jobForm.threshold + jobForm.unit` or something like that - maybe that would be easier to read?

Roger that; sounds like you do have justification for the pack/split. ✔️

Your rename idea is probably a good thought, though. (IIRC you were using the variable threshold in some places to be the unpacked value and in others to be the packed value, which is a bit confusing.)

components/automate-ui/src/app/pages/automate-settings/automate-settings.component.ts

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

…well, could not use formgroup because its actually the value i'm returning Signed-off-by: seajamied <[email protected]>

This refactor makes the function more clear so that it is actually returning just the form as the name implies. We now extract the raw values after the form is returned. Signed-off-by: seajamied <[email protected]>

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

msorens

Great work! Just a few vestigial items.
Let me know if you have any outstanding questions on your end.

components/automate-ui/src/app/entities/automate-settings/automate-settings.model.ts

components/automate-ui/src/app/entities/automate-settings/automate-settings.requests.ts

components/automate-ui/src/app/pages/automate-settings/automate-settings.component.spec.ts

components/automate-ui/src/app/pages/automate-settings/automate-settings.component.ts

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

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

susanev

one small ask, everything else looks great.. thank you!!!

components/automate-ui/src/app/pages/automate-settings/automate-settings.component.ts

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

susanev · 2020-04-02T13:34:36Z

components/automate-ui/src/app/pages/automate-settings/automate-settings.component.spec.ts

        expect(component.notificationType).toEqual('info');
        expect(component.notificationMessage)
-          .toEqual('All settings have been updated successfully');
-        expect(component.notificationVisible).toEqual(true);
+        .toEqual('All settings have been updated successfully');


sorry, shoulda commented on the test too

Suggested change

.toEqual('All settings have been updated successfully');

.toEqual('Settings saved.');

nah, that's not your responsibility - that was on me. I pushed up and went to bed without checking the tests. Sorry about that!

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

SEAjamieD requested a review from susanev March 19, 2020 18:40

SEAjamieD force-pushed the jamie/2523-data-lifecycle-refactor branch 2 times, most recently from 152890d to 2780332 Compare March 19, 2020 23:05

SEAjamieD mentioned this pull request Mar 20, 2020

Jamie/2523 data lifecycle card1 #2967

Closed

4 tasks

SEAjamieD requested review from a team March 20, 2020 01:02

SEAjamieD commented Mar 20, 2020

View reviewed changes

components/automate-ui/src/app/entities/automate-settings/automate-settings.effects.ts Show resolved Hide resolved

SEAjamieD commented Mar 20, 2020

View reviewed changes

components/automate-ui/src/app/entities/automate-settings/automate-settings.requests.ts Outdated Show resolved Hide resolved

SEAjamieD commented Mar 20, 2020

View reviewed changes

components/automate-ui/src/app/entities/automate-settings/automate-settings.requests.ts Outdated Show resolved Hide resolved

SEAjamieD commented Mar 20, 2020

View reviewed changes

SEAjamieD self-assigned this Mar 20, 2020

SEAjamieD added automate-ui ui labels Mar 20, 2020

susanev mentioned this pull request Mar 20, 2020

Adding IAM Actions doc #3157

Merged

4 tasks

susanev reviewed Mar 20, 2020

View reviewed changes

SEAjamieD requested a review from a team March 20, 2020 19:15

msorens reviewed Mar 20, 2020

View reviewed changes

msorens reviewed Mar 21, 2020

View reviewed changes

susanev mentioned this pull request Mar 22, 2020

data lifecycle doc updates #3170

Merged

12 tasks

msorens reviewed Mar 23, 2020

View reviewed changes

msorens reviewed Mar 28, 2020

View reviewed changes

seajamied added 11 commits March 30, 2020 19:51

Update retention api urls

9db1cb9

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

Add FormGroup as a type

40f5ba1

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

Add private function for disabing/enabling form controls

65b7e1f

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

Shorten GetJobForm function, no type checking on it yet

c869dd2

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

Add function signature to splitThreshold

69b3975

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

Add function signature to getJobForm, created an interface for it as …

411d631

…well, could not use formgroup because its actually the value i'm returning Signed-off-by: seajamied <[email protected]>

Refactor getJobForm

0da2ee4

This refactor makes the function more clear so that it is actually returning just the form as the name implies. We now extract the raw values after the form is returned. Signed-off-by: seajamied <[email protected]>

Formatting to shrink up the html lines taken

275b462

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

Update model to accept validators, add validation on to all text inputs

8c6d7da

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

spelling

a77dae5

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

spelling

f8886b7

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

msorens approved these changes Apr 1, 2020

View reviewed changes

seajamied added 9 commits April 1, 2020 12:55

Fix that grey background, turns out two elements needed to be white t…

3749de4

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

Lint fix

67920a1

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

Spelling corrections

7d0735d

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

Spelling and comment corrections

b2086c5

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

Remove getJobForm function - only needed in one spot

f2b3b1d

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

Update tests to separate out nested and non-nested into separate tests

b1bd7bf

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

Add needed types in model

9695ee0

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

Adding console.errors in the strange case that a name didn't exist

8844ef4

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

More commenting for clarity

2e72877

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

susanev approved these changes Apr 2, 2020

View reviewed changes

components/automate-ui/src/app/pages/automate-settings/automate-settings.component.ts Outdated Show resolved Hide resolved

Update sentence for UX

ebbd42e

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

susanev reviewed Apr 2, 2020

View reviewed changes

Updated test for UX change

3848132

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

SEAjamieD merged commit 2bfded1 into master Apr 2, 2020

chef-expeditor bot deleted the jamie/2523-data-lifecycle-refactor branch April 2, 2020 15:53

susanev mentioned this pull request Apr 23, 2020

fix data lifecycle client run docs #3487

Merged

4 tasks

msorens mentioned this pull request Jul 2, 2020

Light up typescript check: strictDomLocalRefTypes #4028

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Jamie/2523 data lifecycle refactor #3158

Jamie/2523 data lifecycle refactor #3158

SEAjamieD commented Mar 19, 2020 •

edited

Loading

SEAjamieD commented Mar 19, 2020

SEAjamieD commented Mar 20, 2020

SEAjamieD Mar 20, 2020

SEAjamieD Mar 20, 2020

SEAjamieD Mar 20, 2020

susanev left a comment •

edited

Loading

susanev commented Mar 20, 2020

susanev commented Mar 20, 2020

SEAjamieD commented Mar 20, 2020

SEAjamieD commented Mar 20, 2020 •

edited

Loading

msorens left a comment

msorens Mar 20, 2020

SEAjamieD Mar 26, 2020

mjingle Mar 26, 2020

susanev Mar 26, 2020

SEAjamieD Mar 26, 2020

susanev commented Mar 21, 2020

msorens left a comment

susanev commented Mar 22, 2020

msorens Mar 27, 2020

SEAjamieD Mar 31, 2020

msorens Apr 1, 2020

msorens left a comment

susanev left a comment

susanev Apr 2, 2020

SEAjamieD Apr 2, 2020

	.toEqual('All settings have been updated successfully');
	.toEqual('Settings saved.');

Jamie/2523 data lifecycle refactor #3158

Jamie/2523 data lifecycle refactor #3158

Conversation

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

SEAjamieD commented Mar 19, 2020

SEAjamieD commented Mar 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

susanev left a comment • edited Loading

Choose a reason for hiding this comment

susanev commented Mar 20, 2020

susanev commented Mar 20, 2020

SEAjamieD commented Mar 20, 2020

SEAjamieD commented Mar 20, 2020 • 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

susanev commented Mar 21, 2020

msorens left a comment

Choose a reason for hiding this comment

susanev commented Mar 22, 2020

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

susanev left a 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 19, 2020 •

edited

Loading

susanev left a comment •

edited

Loading

SEAjamieD commented Mar 20, 2020 •

edited

Loading