UI: Update dashboard client count query to use `billing_start_timestamp` #26729

hellobontempo · 2024-05-01T10:05:18Z

When we implemented Census reporting #20125 we stopped querying the /sys/license/status endpoint to instead retrieve the billing_start_timestamp from sys/internal/counters/config

This PR updates the UI dashboard to follow the same pattern so there is parity in the start_time used to make the request to the activity log. When the license query was removed in the client count refactor, we also stopped using local storage to save inputted dates so vault:ui-inputted-start-date is irrelevant now.

github-actions · 2024-05-01T10:10:09Z

Build Results:
All builds succeeded! ✅

github-actions · 2024-05-01T10:10:26Z

CI Results:
All Go tests succeeded! ✅

hellobontempo · 2024-05-01T10:34:15Z

ui/app/components/dashboard/client-count-card.js

+    if (!this.clientConfig) {
+      // set config and license start time when component initializes
+      this.clientConfig = yield this.store.queryRecord('clients/config', {}).catch(() => {});
+      this.licenseStartTime = setStartTimeQuery(this.args.isEnterprise, this.clientConfig);


technically I could just pass true here instead of this.args.isEnterprise because this component is wrapped in a conditional and only renders for enterprise versions. however, in case a future refactor shuffled conditionals, I wanted to preserve the logic in setStartTimeQuery which prevents from accidentally setting the billing start time to 0001-01-01T00:00:00Z for community versions

hellobontempo · 2024-05-01T12:44:07Z

ui/app/components/dashboard/client-count-card.js

  }

  get currentMonthActivityTotalCount() {
    return this.activityData?.byMonth?.lastObject?.new_clients.clients;
  }

-  get licenseStartTime() {
-    return this.args.license.startTime || getStorage().getItem('vault:ui-inputted-start-date') || null;


we no longer saved the inputted user start date in local storage

hellobontempo · 2024-05-01T12:48:34Z

ui/app/routes/vault/cluster/clients/counts.ts

-    if (this.version.isEnterprise && this._hasConfig(config)) {
-      startTime = getUnixTime(config.billingStartTimestamp);
-    }
+    const startTime = setStartTimeQuery(this.version.isEnterprise, config);


Although this logic is fairly simple and only used in two places, I wanted to consolidate it so that setting the start time was handled the same in both places the /activity log is queried. See Jira VAULT-25315 for more context around the bug introduced by not checking the version.

hellobontempo · 2024-05-01T12:52:33Z

ui/app/components/dashboard/client-count-card.hbs

@@ -55,7 +55,7 @@
        />
        <small class="has-left-margin-xs has-text-grey">
          Updated
-          {{date-format this.updatedAt "MMM dd, yyyy hh:mm:ss"}}
+          {{date-format this.updatedAt "MMM d yyyy, h:mm:ss aaa" withTimeZone=true}}


This format is consistent with the format we show response timestamps elsewhere in the app

andaley

nice! i think moving this into a util to ensure the same start time is applied in both areas is a great solution. only request i have here is tests -- could we get some coverage for setStartTimeQuery in the client-count-utils-test? it'd be good to test for community, ent, an empty config, a populated config etc.

other than that looks great 🙌

andaley · 2024-05-01T13:57:48Z

ui/app/components/dashboard/client-count-card.js

- * ```
- *  @param {object} license - license object passed from the parent
+ *
+ * <Dashboard::ClientCountCard />


Suggested change

* <Dashboard::ClientCountCard />

* <Dashboard::ClientCountCard @isEnterprise={{@version.isEnterprise}} />

looks like we're missing that arg 🙂

Yeah! I added that last minute - thanks!

hellobontempo · 2024-05-01T16:18:48Z

ui/tests/integration/components/dashboard/overview-test.js

@@ -140,43 +149,11 @@ module('Integration | Component | dashboard/overview', function (hooks) {
    assert.dom(DASHBOARD.cardName('client-count')).exists();
  });

-  test('it should hide client count on enterprise w/o license ', async function (assert) {


this test didn't really make sense because enterprise versions don't exist without a license

andaley · 2024-05-01T16:31:38Z

ui/tests/integration/components/dashboard/client-count-card-test.js

+    assert.expect(4);
+    this.server.get('sys/internal/counters/activity', () => {
+      // this assertion should NOT be hit in this test
+      assert.true(true, 'uh oh! makes request to sys/internal/counters/activity');


Suggested change

assert.true(true, 'uh oh! makes request to sys/internal/counters/activity');

throw new Error('uh oh! makes request to sys/internal/counters/activity');

^ another way to more clearly verify that the endpoint isn't called in this test. throwing an error will cause the test to fail. if you take this approach, you'll also want to adjust the assert.expect(4) to 3

Great idea! Much clearer 💡

andaley · 2024-05-01T16:42:19Z

ui/tests/integration/utils/client-count-utils-test.js

+    assert.strictEqual(setStartTimeQuery(true, {}), null, `it returns null if no permission to ${apiPath}`);
+    assert.strictEqual(
+      setStartTimeQuery(false, {}),
+      null,
+      `it returns null for community edition and no permission to ${apiPath}`
+    );
+    assert.strictEqual(
+      setStartTimeQuery(true, { billingStartTimestamp: new Date('2022-06-08T00:00:00Z') }),
+      1654646400,
+      'it returns unix time if enterprise and billing_start_timestamp exists'
+    );
+    assert.strictEqual(
+      setStartTimeQuery(false, { billingStartTimestamp: new Date('0001-01-01T00:00:00Z') }),
+      null,
+      'it returns null time for community edition even if billing_start_timestamp exists'
+    );
+    assert.strictEqual(
+      setStartTimeQuery(false, { foo: 'bar' }),
+      null,
+      'it returns null if billing_start_timestamp key does not exist'


✨ these assertions are normally things i'd split into separate tests within a module. for example:

module('#setStartTimeQuery', function (hooks) { test('community: it returns null with no permission to apiPath', async function (assert) { ... }); test('community: it returns null when billing_start_time_exists', async function (assert) { ... }); test('enterprise: it returns null when billing_start_time_exists', async function (assert) { ... } });

the main benefits of splitting them up this way are that it keeps the tests more isolated (and thus less flaky since there's no chance of the state of one assertion affecting the next one). it also makes them easier to read and debug (since you know exactly which situation failed).

not a blocker, since i know there's plenty of changes in this PR already but it's a pattern i'd encourage us to try out more 🙌

That's a great idea! I'll keep this as is because it's how all of the tests in this file are formatted right now. I think I'd rather update all of them to be separate modules at once. 🤔

Great feedback, though. I'll keep this in mind when writing new tests and add a todo comment to the top of this file to keep it in mind as a future improvement.

…-query

hellobontempo added 2 commits May 1, 2024 09:58

remvoe request tolicense in dashboard client count card

2b55627

cleanup jsdoc

fbf7706

hellobontempo requested a review from a team as a code owner May 1, 2024 10:05

hellobontempo added the ui label May 1, 2024

github-actions bot added the hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed label May 1, 2024

hellobontempo added pr/no-changelog and removed hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed labels May 1, 2024

hellobontempo added this to the 1.17.0-rc milestone May 1, 2024

hellobontempo removed the pr/no-changelog label May 1, 2024

add changelog

80dd3ed

github-actions bot added the hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed label May 1, 2024

use helper to set start time

5353fe4

hellobontempo commented May 1, 2024

View reviewed changes

andaley suggested changes May 1, 2024

View reviewed changes

hellobontempo added 3 commits May 1, 2024 16:58

update component tests

6542107

update overview test

0d645e0

update util tests

6797d3b

hellobontempo commented May 1, 2024

View reviewed changes

andaley reviewed May 1, 2024

View reviewed changes

andaley approved these changes May 1, 2024

View reviewed changes

hellobontempo added 3 commits May 2, 2024 10:18

throw error instead, add comment to util file

3041dea

fix accidentally removed type from import

2364f05

remove typo arg from test component

a5753a4

vercel bot deployed to Preview May 2, 2024 09:49 View deployment

hellobontempo force-pushed the ui/VAULT-25388/update-dashboard-client-count-query branch from ce15b23 to a5753a4 Compare May 2, 2024 12:50

Merge branch 'main' into ui/VAULT-25388/update-dashboard-client-count…

6697ea2

…-query

vercel bot deployed to Preview May 2, 2024 13:18 View deployment

rename token stat getter to avoid future typos

22f2ed8

hellobontempo enabled auto-merge (squash) May 2, 2024 14:46

hellobontempo disabled auto-merge May 2, 2024 16:04

Merge branch 'main' into ui/VAULT-25388/update-dashboard-client-count…

f1162ee

…-query

vercel bot deployed to Preview May 2, 2024 16:11 View deployment

hellobontempo enabled auto-merge (squash) May 2, 2024 17:35

fix tests, add fallback for util

d3fefc4

hellobontempo merged commit 6d0e4f6 into main May 2, 2024
31 checks passed

hellobontempo deleted the ui/VAULT-25388/update-dashboard-client-count-query branch May 2, 2024 17:45

hellobontempo mentioned this pull request May 7, 2024

UI: wrap client count card in permission conditional #26848

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UI: Update dashboard client count query to use `billing_start_timestamp` #26729

UI: Update dashboard client count query to use `billing_start_timestamp` #26729

hellobontempo commented May 1, 2024

github-actions bot commented May 1, 2024 •

edited

Loading

github-actions bot commented May 1, 2024 •

edited

Loading

hellobontempo May 1, 2024

hellobontempo May 1, 2024

hellobontempo May 1, 2024 •

edited

Loading

hellobontempo May 1, 2024 •

edited

Loading

andaley left a comment

andaley May 1, 2024

hellobontempo May 1, 2024

hellobontempo May 1, 2024

andaley May 1, 2024 •

edited

Loading

hellobontempo May 1, 2024

andaley May 1, 2024

hellobontempo May 1, 2024

	* <Dashboard::ClientCountCard />
	* <Dashboard::ClientCountCard @isEnterprise={{@version.isEnterprise}} />

	assert.true(true, 'uh oh! makes request to sys/internal/counters/activity');
	throw new Error('uh oh! makes request to sys/internal/counters/activity');

UI: Update dashboard client count query to use billing_start_timestamp #26729

UI: Update dashboard client count query to use billing_start_timestamp #26729

Conversation

hellobontempo commented May 1, 2024

github-actions bot commented May 1, 2024 • edited Loading

github-actions bot commented May 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hellobontempo May 1, 2024 • edited Loading

Choose a reason for hiding this comment

hellobontempo May 1, 2024 • edited Loading

Choose a reason for hiding this comment

andaley 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

andaley May 1, 2024 • 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

UI: Update dashboard client count query to use `billing_start_timestamp` #26729

UI: Update dashboard client count query to use `billing_start_timestamp` #26729

github-actions bot commented May 1, 2024 •

edited

Loading

github-actions bot commented May 1, 2024 •

edited

Loading

hellobontempo May 1, 2024 •

edited

Loading

hellobontempo May 1, 2024 •

edited

Loading

andaley May 1, 2024 •

edited

Loading