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

Add disease specific AOE tab for bulk upload guide #8165

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

mpbrown
Copy link
Collaborator

@mpbrown mpbrown commented Oct 1, 2024

FRONTEND PULL REQUEST

Related Issue

Changes Proposed

  • Adds disease tab feature to CSV documentation sections
  • Converts CsvSchemaItem to use a RequiredStatusTag enum instead of combinations of required and requested
  • Removes requested in favor of optional

Additional Information

  • Ignore the "Duplicated Lines" warning from SonarLint since we are defining the documentation schema data

Testing

  • Deployed on dev6

Screenshots / Demos

image

image

image

);
const user = userEvent.setup();

expect(screen.getAllByText("Required for Positives")).toHaveLength(1);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

⚠️ expecting it to have length 1 because we use the Required for Positives tag in the explanation of how the tags work at the start of the guide

{
title: MULTIPLEX_DISEASES.HIV,
slug: MULTIPLEX_DISEASES.HIV,
items: [
Copy link
Collaborator

Choose a reason for hiding this comment

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

In this PR are we addressing the HIV AOE questions or just adding the disease specific AOE tabs?

Because if we want the guide to reflect what is required we should add genders of sexual partners.

Let me know what you think! 👀

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just adding disease specific AOE tabs for this ticket, but #7461 is in the parking lot for adding the content for new AOE like genders of sexual partners

Copy link
Collaborator

@emyl3 emyl3 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 on the implementation of this, Mike! I left a couple comments/questions for you!

emyl3
emyl3 previously approved these changes Oct 7, 2024
Copy link
Collaborator

@emyl3 emyl3 left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you!!

@mehansen
Copy link
Collaborator

mehansen commented Oct 8, 2024

I can't tell from the original ticket, but did we want the non-relevant AOE questions for a disease to even show in that tab? e.g. employed_in_healthcare is not something receivers are even interested in for HIV, and it's not part of the AOE on the test card for HIV devices, so I would think we don't even need to show it as an AOE question for HIV

bobbywells52
bobbywells52 previously approved these changes Oct 8, 2024
Copy link
Collaborator

@bobbywells52 bobbywells52 left a comment

Choose a reason for hiding this comment

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

LGTM thanks for your great work on this!

@mpbrown mpbrown dismissed stale reviews from bobbywells52 and emyl3 via cfadc68 October 8, 2024 19:54
@mpbrown
Copy link
Collaborator Author

mpbrown commented Oct 8, 2024

@mehansen I checked with Jayna and we're good to remove those AOE columns for HIV, so I just updated it here

image

@emyl3
Copy link
Collaborator

emyl3 commented Oct 9, 2024

I can't tell from the original ticket, but did we want the non-relevant AOE questions for a disease to even show in that tab? e.g. employed_in_healthcare is not something receivers are even interested in for HIV, and it's not part of the AOE on the test card for HIV devices, so I would think we don't even need to show it as an AOE question for HIV

(unrelated to your changes @mpbrown 😅)
Even though the receivers aren't interested, don't we technically still send employed_in_healthcare and resident_congregate_setting within patient_data for single entry tests regardless of disease? Is this something we need to clean up? 🤔

We also have some columns like icu, residence_type, and hospitalized that (I think...) we no longer ask for or send on the single entry side of things, is this something we also need to clean up/align with our bulk results upload? 🧐

Copy link
Collaborator

@emyl3 emyl3 left a comment

Choose a reason for hiding this comment

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

LGTM! Tested locally!

@mehansen
Copy link
Collaborator

I can't tell from the original ticket, but did we want the non-relevant AOE questions for a disease to even show in that tab? e.g. employed_in_healthcare is not something receivers are even interested in for HIV, and it's not part of the AOE on the test card for HIV devices, so I would think we don't even need to show it as an AOE question for HIV

(unrelated to your changes @mpbrown 😅) Even though the receivers aren't interested, don't we technically still send employed_in_healthcare and resident_congregate_setting within patient_data for single entry tests regardless of disease? Is this something we need to clean up? 🤔

We also have some columns like icu, residence_type, and hospitalized that (I think...) we no longer ask for or send on the single entry side of things, is this something we also need to clean up/align with our bulk results upload? 🧐

@emyl3 Ooh these are great questions. We do send that data for all single-entry. IMO I think it's okay to send it and it won't hurt, but I don't know if I would categorize it as an HIV AOE question. However it's still valid data to be potentially included so maybe it does belong in the bulk upload guide somewhere? Another one potentially like that is patient sex assigned at birth.

Re: the fields that we only collect for bulk upload, I wouldn't mind if we stopped collecting them because from an engineering standpoint it makes maintenance either but I'm not sure whether we have product reasons for keeping them around.

cc @jayna-SkylightDigital to get her thoughts

@mpbrown
Copy link
Collaborator Author

mpbrown commented Oct 11, 2024

Rebasing this PR for the new terraform changes, so I redeployed to dev6 and it's ready for re-review again!

Copy link

sonarcloud bot commented Oct 11, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
12.5% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

Copy link
Collaborator

@emyl3 emyl3 left a comment

Choose a reason for hiding this comment

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

🚢

Copy link
Collaborator

@mehansen mehansen left a comment

Choose a reason for hiding this comment

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

lgtm! thanks for this

Comment on lines -17 to -25
export interface Item {
name: string;
colHeader: string;
required: boolean;
requested: boolean;
examples?: string[];
description?: string[];
acceptedValues?: string[];
format?: string;
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice consolidation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bulk Upload Guide] Add Disease-specific labels
4 participants