Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UI: Use Client Count export API #27455

Merged
merged 44 commits into from
Aug 1, 2024
Merged

Conversation

hashishaw
Copy link
Contributor

@hashishaw hashishaw commented Jun 11, 2024

Description

This PR updates the Client Count Dashboard to use the dedicated export API. Relies on enterprise PR 6171

  • Copy for the export modal was updated to reflect the new source of data
  • Added loading state to modal when export is running
  • Added a dropdown to the modal for the user to choose the format
  • The current namespace + selected namespace is added to the export header so that the downloaded data reflects the filters on the page
  • Export button is hidden for users without sudo capabilities to export at the given namespace

Kapture 2024-07-10 at 14 49 58

Updated modal content
updated copy
loading

Updated CSV content
Screenshot 2024-06-12 at 12 26 00

Previous CSV content
Screenshot 2024-06-12 at 12 27 04

  • Ent tests pass

@hashishaw hashishaw added the ui label Jun 11, 2024
@hashishaw hashishaw added this to the 1.18.0-rc milestone Jun 11, 2024
@github-actions github-actions bot added the hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed label Jun 11, 2024
Copy link

github-actions bot commented Jun 11, 2024

CI Results:
All Go tests succeeded! ✅

@hashishaw hashishaw marked this pull request as ready for review June 11, 2024 22:40
@hashishaw hashishaw requested a review from a team as a code owner June 11, 2024 22:40
Copy link

github-actions bot commented Jun 11, 2024

Build Results:
All builds succeeded! ✅

Copy link
Contributor

@hellobontempo hellobontempo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a comment about a conditional. Great work!

ui/app/utils/query-param-string.js Outdated Show resolved Hide resolved
Copy link
Contributor

@hellobontempo hellobontempo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a screenshot of how the new export csv data looks?

@hashishaw hashishaw marked this pull request as draft June 20, 2024 20:53
Copy link
Contributor

@hellobontempo hellobontempo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking great! Just a couple comments/questions for now! Will continue reviewing once tests updates are complete 😄

ui/app/components/clients/attribution.js Show resolved Hide resolved
ui/app/components/clients/attribution.js Show resolved Hide resolved
async generateCsvData() {
const adapter = this.store.adapterFor('clients/activity');
const currentNs = this.namespace.path;
const { startTimestamp, endTimestamp, selectedNamespace } = this.args;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CC work has had so many iterations 🫠 - these start/end timestamps update when the date params change, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct!

return `This export will include the namespace path, mount path and associated total entity, non-entity${
isSecretsSyncActivated ? ', ACME and secrets sync clients' : ' and ACME clients'
} for the ${this.formattedEndDate ? 'date range' : 'month'} below.`;
exportChartData = task({ drop: true }, async (filename) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a difference between using this syntax vs:

  @task({ drop: true })
  *exportChartData(filename) {...}

(aside from this changing the await below to a yield)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I used is the latest recommended syntax -- in my experience, it also has a better TS experience (although that is not relevant here) https://ember-concurrency.com/docs/task-concurrency

*/

/**
* queryParamString converts an object to a query param string with URL encoded keys and values.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great idea making this a util!

import queryParamString from 'vault/utils/query-param-string';
import { module, test } from 'qunit';

module('Unit | Utility | query-param-string', function () {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏 👏 👏

return resp.blob();
}
// If it's an empty response (eg 204), there's no data so return an error
errorMsg = 'no data to export in provided time range.';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: caps 'no' -> 'No'

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm matching the casing of the API error messages, which don't have capitalization

Copy link
Contributor

@Monkeychip Monkeychip left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤩 nice work!

@hashishaw hashishaw merged commit 10068ff into main Aug 1, 2024
31 checks passed
@hashishaw hashishaw deleted the ui/VAULT-27595/use-cc-export-api branch August 1, 2024 16:03
Monkeychip pushed a commit that referenced this pull request Aug 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants