-
Notifications
You must be signed in to change notification settings - Fork 6
[DIT-11721] Add support for filtering by status in configs. #128
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
[DIT-11721] Add support for filtering by status in configs. #128
Conversation
marla-hoggard
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From a quick scan, things look good! But i'm going to reassign because I've done a ton of reviews this week and need some heads down time. @bparrish17 and @joustrich are also working on the CLI right now, so I'll re-assign to one of them, in case there's any conflicts worth noting.
As a reminder, you'll also need to open up a separate PR in the main ditto repo with docs updates + changelog after this PR goes in and the new CLI version is released.
It would probably be a good idea to include a version bump of the CLI version that we use in our own codebase in that same PR, so that we always have the latest version. I'll add that to the notion doc.
bparrish17
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just one request to add statuses config support to outputs as well!
| let filters: PullFilters = { | ||
| projects: this.projectConfig.projects, | ||
| variants: this.projectConfig.variants, | ||
| statuses: this.projectConfig.statuses |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be available on the output filters as well, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lib/src/http/types.ts
Outdated
| text: z.string(), | ||
| richText: z.string().optional(), | ||
| status: z.string(), | ||
| status: ZTextStatus, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
weird this was string originally!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually it might be better to keep it as string for now. It makes changes to the status schema easier (does not require an update to the CLI to fetch new types of statuses) Especially if the status is essentially "unused" from a typing perspective on output

Overview
Provide parity to the CLI with the Dev Api by including the ability to filter textItems and Components by status when pulling text from Ditto.
Note: This follows the same logic for filtering variants as the fetch textItem and fetch components endpoints. Variants will only be fetched when they match the status filter and their corresponding base text also matches the status filter.
Context
Support for filtering components/textItems by status was added to the Developer api. We are adding support to the cli so that there is parity in how pull operates between the two.
https://developer.dittowords.com/api-reference/text-items/get-text-items
https://developer.dittowords.com/api-reference/components/get-components
Screenshots
Sample Component (Base is
WIPvariants areWIPorREVIEW)Sample textItem (Base is
WIP)Sample textItem (Variants are
WIPorFINAL)pullusing the above sample data,allvariants andFINALandWIPstatus filter returns all variants of the textItem and the base component andhobbitishvariant. Thetest1variant component is not returned as it has aREVIEWstatus.pullusingallvariants andFINALstatus filter returns no textItems or components. While thehobbitishtextItem has aFINALstatus, its base text isWIPso it will not be fetched.pullusing the above sample data,allvariants andREVIEWandWIPstatus filter returns all variants of the componet and the base textItem andtest1variant. Thehobbitishvariant textItem is not returned as it has aFINALstatus.pullusinghobbitishvariant andREVIEWandWIPstatus filter returns onlyhobbitishcomponent variant. Thehobbitishvariant textItem is not returned as it has aFINALstatus.pullusinghobbitishvariant andREVIEW,FINAL, andWIPstatus filter returns thehobbitishboth component and textItem variants.pullusinghobbitishvariant andREVIEWandFINALstatus filter returns nothing, because neither component nor textItem variants have a base text that matches the status filter.pullwith no status filter or empty status filter defaults to allowing all statuses to be included.Using an invalid status will display a message informing of the type issue.
Test Plan
[] Run pull using
yarn sync[] filtering without statuses should give all textItems/components back
[] filtering with statuses as [] should also return all textItems/components
[] filtering by a specific status WIP or FINAL should only return those items/components
[] Using status filters with variant filter should only return textItems/components that match the variant and status filters and have base text that also matches the status filter. (Only return variants who match the filters from the set of variants that have base text that matches the project and status filters. )