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

Evidence CMS general updates #12638

Merged
merged 9 commits into from
Dec 11, 2024
Merged

Conversation

emilia-friedberg
Copy link
Member

@emilia-friedberg emilia-friedberg commented Dec 9, 2024

WHAT

Updates to the Evidence CMS to hide AutoML columns when the ai_type is not auto_ml, and fix a bug where filtering rules by conjunction didn't work.

WHY

These are parts of cards 9 and 10 that can be completed before the new rules themselves are in place.

HOW

Mostly just React updates. I did take the opportunity to add more extensive tests to the components I was changing, and also set the timezone for all jest tests so I stop encountering mismatched snapshots between my local env and CircleCI (because we have historically gotten around this by mocking the time in individual tests, it only updated a small handful of snapshots).

Screenshots

(If applicable. Also, please censor any sensitive data)

Notion Card Links

https://www.notion.so/quill/GenAI-Rules-153d42e6f94180679b48f774351ac63e?pvs=4#157d42e6f941808d8e56da0069d8d430

What have you done to QA this feature?

Tested on staging, and Jamie will as well

PR Checklist Your Answer
Have you added and/or updated tests? YES
Have you deployed to Staging? YES
Self-Review: Have you done an initial self-review of the code below on Github? YES

@emilia-friedberg emilia-friedberg marked this pull request as ready for review December 9, 2024 17:47
Copy link
Contributor

@anathomical anathomical 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 good. I like a lot of the stream-lining you've done here, and with one exception, it was all super-intuitive to read through and understand.

@@ -181,7 +181,7 @@
"jscs": "jscs --verbose .",
"jscs:fix": "jscs --verbose -x . --silent",
"test": "npm run build:test && rspec",
"jest": "node --expose-gc ./node_modules/.bin/jest --maxWorkers=1 --logHeapUsage",
"jest": "TZ=utc node --expose-gc ./node_modules/.bin/jest --maxWorkers=1 --logHeapUsage",
Copy link
Contributor

Choose a reason for hiding this comment

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

Good call on this update.


const formattedRows = sortedRules && sortedRules.map((rule: RuleInterface, i: number) => {
const formattedRows = sortedAndFilteredRules && sortedAndFilteredRules.map((rule: RuleInterface, i: number) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I see that you're just modifying the old code pattern, but I think we could clean this up with a safe-accessor.

const formattedRows = sortedAndFilteredRules?.map((rule: RuleInterface, i: number) => {

{/* eslint-disable-next-line react/jsx-no-bind */}
<Route path='/activities/:activityId/rules-index/because' render={() => <Rules activityId={activityId} history={history} prompt={getPromptForComponent(activityData, BECAUSE)} />} />
<Route path='/activities/:activityId/rules-index/because' render={() => <Rules activityId={activityId} history={history} prompt={getPromptArrayForComponent(activityData, BECAUSE)[0]} />} />
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't actually have a better suggestion here, but I will say that it was not obvious to me why we'd want to reference by [0] on first read, so I can imagine being confused again next time I look at this. Still, it seems reasonable enough to me given the limited scope here and the lack of an obvious (to me) alternative that doesn't feel too heavy (like a new helper function that includes the [0] internally, which feels awkward, too).

Copy link
Member

Choose a reason for hiding this comment

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

I would recommend adding a helper method getPromptForComponent back into promptHelpers. Seeing the [0] in the code begs the question, what are the other elements and do I need to know about them. The helper method can explain why you're pulling the first element.

Copy link
Member

@brendanshean brendanshean left a comment

Choose a reason for hiding this comment

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

I really appreciate the testing cleanup here.

@emilia-friedberg emilia-friedberg merged commit b61c84c into develop Dec 11, 2024
17 checks passed
@emilia-friedberg emilia-friedberg deleted the fix/filter-rules-based-on-prompt branch December 11, 2024 14:41
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.

4 participants