-
Notifications
You must be signed in to change notification settings - Fork 5
Implement Shared Eligibility Checker #3269
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
base: main
Are you sure you want to change the base?
Implement Shared Eligibility Checker #3269
Conversation
…nce frequency change handling
…andling for billing periods
…ne rate plan ID retrieval
…ine rate plan ID retrieval
…cy change handling
…valid billing period test
…s in frequency change process
…productCatalog parameter and improve error handling
…o streamline target rate plan key determination
… streamline product details retrieval
…for billing impact assessment
…ing Zuora Orders API
…d enhance error handling
…ility in frequency change processing
…fore accessing rate plans
…r handling in rate plan lookup
…ssary whitespace in getTargetRatePlanId function
… and execution scenarios
…uency change handling
…by filtering invoice items
…cySwitch function
…handling in processFrequencySwitch function
…cy switch process
…nd update related logic
…dation for frequency switch process
…annual billing switch
…ng chargedThroughDate
…ing catalog ID for accuracy
…n processFrequencySwitch function
…plans during frequency switch process
…lanOrderAction type
…uency switch process
…ce in frequency switch process
…ing unused product catalog helper and directly accessing the Annual rate plan
…solidating preview and execution logic, enhancing error handling, and improving code readability
…requirements and integrate into frequency switch process
| "@modules/product-catalog/*": ["./modules/product-catalog/src/*"], | ||
| "@modules/promotions/*": ["./modules/promotions/src/*"], | ||
| "@modules/email/*": ["./modules/email/src/*"], | ||
| "@modules/eligibility/*": ["./modules/eligibility/src/*"], |
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.
well done for remembering to put it in here, it's easy to forget
| catalog: | ||
| zod: ^3.23.8 | ||
| typescript: ^5.6.3 | ||
| dayjs: ^1.11.13 |
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.
interesting idea to put it here - I'm not 100% on the benefits of this but it does sound like a good idea.
does that mean we should do a PR to move all the other uses of dayjs over to catalog: as well?
| "fix-formatting": "prettier --write **.ts" | ||
| }, | ||
| "dependencies": { | ||
| "dayjs": "catalog:" |
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.
why is this needed now, as I can't see any obvious change that would need it?
| nextInvoiceGreaterThanZero: 'next invoice total must be greater than zero', | ||
| mustHaveDiscountDefined: 'subscription must have a discount defined', | ||
| }; | ||
| // Re-exporting from shared module for backward compatibility |
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.
good idea to keep the diff clean. It would be nice to have a direct reference in the longer term though.
| @@ -0,0 +1,140 @@ | |||
| import { ValidationError } from '@modules/errors'; | |||
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.
I assume this is just a direct copy/move of the existing file? if not please could you flag any notable changes
johnduffell
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.
great idea, and thanks for splitting it into a separate PR.
I think this PR could probably target main (and be merged first) rather than your branch, which would take some of the pressure off your other branch.
I've approved with a few minor comments/questions (nothing blocking) for you to check
What does this change?
This pull request is based on this suggestion from @johnduffell .
Since this is a big change, this approach is now being displayed as a pull request to keep focus on #3176