-
Notifications
You must be signed in to change notification settings - Fork 68
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
feat: add db schema and types for Plan API #1788
Conversation
965b7b3
to
0f4093d
Compare
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.
Nice!
} | ||
} | ||
|
||
func JSONStringValueScanner[T any]() field.ValueScannerFunc[T, *sql.NullString] { |
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'd suggest to move this under pkg/framework
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.
Done.
// Equal is a relaxed version of StrictEqual where it is allowed to return true in case a subset of the attributes are equal. | ||
// Typically, you want to use Equal to compare two instances of T where one or both missing some metadata which are | ||
// not required for comparison. Like comparing two instances of T before and after stored in data layer which assigns | ||
// additional metadata like timestamps, unique identifiers, etc. | ||
Equal(T) bool |
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.
just an opinion, but I'm not sure I'm sold on this... if this behaves differently case-by-case then IMO it should be a different thing altogether
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'll remove it for now and we can discuss it separately. This is not required for this PR.
0f4093d
to
48e7282
Compare
} | ||
|
||
func (Plan) Mixin() []ent.Mixin { | ||
return []ent.Mixin{ |
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.
we have the UniqueResourceMixin
to combine these:
type UniqueResourceMixin struct { |
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.
Done.
Default("USD"). | ||
NotEmpty(). | ||
Immutable(), | ||
field.Time("effectiveFrom"). |
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.
we usually use underscore to define ent schema
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.
Agreed. Copy/paste error. Will fix it now.
|
||
func (PlanPhase) Mixin() []ent.Mixin { | ||
return []ent.Mixin{ | ||
entutils.IDMixin{}, |
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.
UniqueResource?
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.
Done.
Optional(). | ||
Nillable(), | ||
field.String("discounts"). | ||
GoType([]plan.Discount{}). |
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.
normalize please, separate PR is ok
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.
Hmm. Why do we need the discounts to be normalized here?
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.
@hekike please discuss with @tothandras, the two of you are on a different stance regarding normalization so IMO lets come up with an agreement (came up on other PRs as well)
|
||
func (PlanRateCard) Mixin() []ent.Mixin { | ||
return []ent.Mixin{ | ||
entutils.IDMixin{}, |
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.
UniqueResource?
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.
Done.
field.String("description"). | ||
Optional(). | ||
Nillable(), | ||
field.String("feature_key"). |
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.
these are mutable because user can edit plan in draft, right?
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.
Yup, thats the idea.
48e7282
to
7b2a254
Compare
7ad710b
to
a7bc011
Compare
a7bc011
to
2e1a0a5
Compare
2e1a0a5
to
a71553c
Compare
Overview