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

[Agency Dashboard] Create agency data store to fetch published data #189

Merged
merged 9 commits into from
Dec 2, 2022

Conversation

terryttsai
Copy link
Contributor

@terryttsai terryttsai commented Nov 30, 2022

Description of the change

Create new AgencyDataStore that fetches and stores from the agency published_data endpoint.

In this PR, this store is being used to properly propagate metric names (no more seeing metric keys instead of metric names if no data is published for a metric!)

The store also completely replaces the old agency dashboard datapointsStore, so I am removing it in this PR!

Type of change

All pull requests must have at least one of the following labels applied (otherwise the PR will fail):

Label Description
Type: Bug non-breaking change that fixes an issue
Type: Feature non-breaking change that adds functionality
Type: Breaking Change fix or feature that would cause existing functionality to not work as expected
Type: Non-breaking refactor change addresses some tech debt item or prepares for a later change, but does not change functionality
Type: Configuration Change adjusts configuration to achieve some end related to functionality, development, performance, or security
Type: Dependency Upgrade upgrades a project dependency - these changes are not included in release notes

Related issues

Closes #188

Checklists

Development

This box MUST be checked by the submitter prior to merging:

  • Double- and triple-checked that there is no Personally Identifiable Information (PII) being mistakenly added in this pull request

These boxes should be checked by the submitter prior to merging:

  • Tests have been written to cover the code changed/added as part of this pull request

Code review

These boxes should be checked by reviewers prior to merging:

  • This pull request has a descriptive title and information useful to a reviewer
  • This pull request has been moved out of a Draft state, has no "Work In Progress" label, and has assigned reviewers
  • Potential security implications or infrastructural changes have been considered, if relevant

@terryttsai terryttsai requested a review from a team November 30, 2022 22:02
* containing start_date, end_date and key value pairs for each dimension and their values.
* See the DatapointsByMetric type for details.
*/
get datapointsByMetric(): DatapointsByMetric {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is already inherited from BaseDatapointsStore, wonder how this got here

@@ -53,14 +52,6 @@ abstract class DatapointsStore {
this.loading = true;
}

get metricKeyToDisplayName(): { [metricKey: string]: string | null } {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

use agency data store's instead

@terryttsai terryttsai marked this pull request as ready for review November 30, 2022 22:15

import { request } from "../utils/networking";

class AgencyDataStore {
Copy link
Contributor Author

@terryttsai terryttsai Nov 30, 2022

Choose a reason for hiding this comment

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

I'm calling it AgencyDataStore because in the future this will include not just an agency's published metric datapoints and metric information but also agency metadata such as name, description etc

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh I love that! Good thinking

* containing start_date, end_date and key value pairs for each dimension and their values.
* See the DatapointsByMetric type for details.
*/
get datapointsByMetric(): DatapointsByMetric {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is similar to datapointsByMetric in the BaseDatapointsStore class, in that it transforms the data in the same way

@terryttsai
Copy link
Contributor Author

We could, either in this PR or a follow-up, delete the shared BaseDatapointsStore class + delete the published_datapoints endpoint

@@ -87,12 +55,10 @@ test("renders list of metrics", async () => {
/Click on a metric to view chart:/i
);
expect(textElement1).toBeInTheDocument();
const textElement2 = await screen.findByText(/LAW_ENFORCEMENT_ARRESTS/i);
const textElement2 = await screen.findByText(/Total Arrests/i);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: might be more helpful to have the variable names closely match the text its locating?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

like textElementTotalArrests? textElementClickOnAMetricToViewChart?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, something like that - only if it feels helpful!

} catch (error) {
showToast("Error fetching data.", false, "red", 4000);
}
};
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can define and use this as is inside the useEffect unless you're using it elsewhere in this component.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call!

navigate(`/agency/${agencyId}/dashboard?metric=${metric}`)
}
onMetricsSelect={(selectedMetricName) => {
const mKey =
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: maybe selectedMetricKey?

@mxosman
Copy link
Contributor

mxosman commented Dec 1, 2022

Terry this looks good to me! Just had a question and some suggestions. I'll let @lilidworkin take a pass as well.

Copy link
Collaborator

@lilidworkin lilidworkin left a comment

Choose a reason for hiding this comment

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

This looks great @terryttsai !

Just to make sure I'm understanding properly, so we are no longer inheriting from BaseDatapointStore in Agency Dashboard, correct? If so, is there still a concern about having to duplicate logic, or is that not relevant anymore?


import { request } from "../utils/networking";

class AgencyDataStore {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh I love that! Good thinking

@@ -37,12 +33,6 @@ class DatapointsStore extends BaseDatapointsStore {

api: API;

rawDatapoints: RawDatapoint[];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just curious, why is it safe to get rid of these fields now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They're already inherited from the BaseDatapointsStore! I'm not sure how those got in there if either I missed them or they got added back in

@lilidworkin
Copy link
Collaborator

We could, either in this PR or a follow-up, delete the shared BaseDatapointsStore class + delete the published_datapoints endpoint

I don't feel strongly about deleting BaseDatapointsStore -- seems safe enough to keep that. But I do think the published_datapoints endpoint should be deleted! I always find it confusing to have deprecated endpoints / APIs in the codebase. Don't feel strongly about whether it happens in this PR or a followup.

@terryttsai
Copy link
Contributor Author

Just to make sure I'm understanding properly, so we are no longer inheriting from BaseDatapointStore in Agency Dashboard, correct? If so, is there still a concern about having to duplicate logic, or is that not relevant anymore?

We don't need to share this logic between the two apps anymore, so there's not really a need for a BaseDatapointsStore since only publisher is using it now

@terryttsai
Copy link
Contributor Author

I'll remove the endpoint in a followup!

@terryttsai terryttsai merged commit 092566f into main Dec 2, 2022
@terryttsai terryttsai deleted the ttt/published_data_api branch December 2, 2022 19:18
helperbot-recidiviz pushed a commit to Recidiviz/pulse-data that referenced this pull request Dec 20, 2022
…diviz-data#17026)

## Description of the change

Remove no-longer-used `published_datapoints` endpoint.

NOTE: must merge Recidiviz/justice-counts#189
before this

## Type of change

> All pull requests must have at least one of the following labels
applied (otherwise the PR will fail):

| Label | Description |
|-----------------------------
|-----------------------------------------------------------------------------------------------------------
|
| Type: Bug | non-breaking change that fixes an issue |
| Type: Feature | non-breaking change that adds functionality |
| Type: Breaking Change | fix or feature that would cause existing
functionality to not work as expected |
| Type: Non-breaking refactor | change addresses some tech debt item or
prepares for a later change, but does not change functionality |
| Type: Configuration Change | adjusts configuration to achieve some end
related to functionality, development, performance, or security |
| Type: Dependency Upgrade | upgrades a project dependency - these
changes are not included in release notes |

## Related issues

Closes #XXXX

## Checklists

### Development

**This box MUST be checked by the submitter prior to merging**:
- [x] **Double- and triple-checked that there is no Personally
Identifiable Information (PII) being mistakenly added in this pull
request**

These boxes should be checked by the submitter prior to merging:
- [ ] Tests have been written to cover the code changed/added as part of
this pull request

### Code review

These boxes should be checked by reviewers prior to merging:

- [ ] This pull request has a descriptive title and information useful
to a reviewer
- [ ] This pull request has been moved out of a Draft state, has no
"Work In Progress" label, and has assigned reviewers
- [ ] Potential security implications or infrastructural changes have
been considered, if relevant

GitOrigin-RevId: 525dab46c78b745f59df4c19d2cb9fac7e00162b
helperbot-recidiviz pushed a commit to Recidiviz/pulse-data that referenced this pull request Apr 19, 2023
…diviz-data#17026)

## Description of the change

Remove no-longer-used `published_datapoints` endpoint.

NOTE: must merge Recidiviz/justice-counts#189
before this

## Type of change

> All pull requests must have at least one of the following labels
applied (otherwise the PR will fail):

| Label | Description |
|-----------------------------
|-----------------------------------------------------------------------------------------------------------
|
| Type: Bug | non-breaking change that fixes an issue |
| Type: Feature | non-breaking change that adds functionality |
| Type: Breaking Change | fix or feature that would cause existing
functionality to not work as expected |
| Type: Non-breaking refactor | change addresses some tech debt item or
prepares for a later change, but does not change functionality |
| Type: Configuration Change | adjusts configuration to achieve some end
related to functionality, development, performance, or security |
| Type: Dependency Upgrade | upgrades a project dependency - these
changes are not included in release notes |

## Related issues

Closes #XXXX

## Checklists

### Development

**This box MUST be checked by the submitter prior to merging**:
- [x] **Double- and triple-checked that there is no Personally
Identifiable Information (PII) being mistakenly added in this pull
request**

These boxes should be checked by the submitter prior to merging:
- [ ] Tests have been written to cover the code changed/added as part of
this pull request

### Code review

These boxes should be checked by reviewers prior to merging:

- [ ] This pull request has a descriptive title and information useful
to a reviewer
- [ ] This pull request has been moved out of a Draft state, has no
"Work In Progress" label, and has assigned reviewers
- [ ] Potential security implications or infrastructural changes have
been considered, if relevant

GitOrigin-RevId: 9fa42847c594e621d7fdd744d360e43ef3c8848c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Agency Dashboard] Use new published_data endpoint to get datapoints
3 participants