Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
FYST-792: ID Grocery Credit #4897
base: main
Are you sure you want to change the base?
FYST-792: ID Grocery Credit #4897
Changes from 45 commits
1f5edf5
2369a7f
b9b7786
4f62674
923d643
039518f
036e43d
925d2f6
c7a22ff
610fae2
f8b1361
e6c7050
9930e40
8333df5
c8f0de1
b41bb7b
ad2a8dd
e34b975
ff426ae
fd42213
bee9d71
f44ae01
7ec2069
3e52f03
aa30894
a878553
9e99260
4f3a7e8
59e05b6
26f771f
2e24471
950cb1c
bebfdee
57f82fa
d2576c3
707ee52
b9eefcf
94f724e
5c2658f
4c2f870
dcc7ba1
ab573cd
5cda731
29c68e1
5933898
c8d4b83
15c6ce9
97d5804
892e5b6
3fe82bc
a534bea
26d9aac
39c616f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 is just a non-related question that I also discussed just now with Drew, but why do we use enum instead of boolean for these types of fields?
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.
Enums let us track yes/no/unfilled instead of just true/false, which I think is primarily useful when generating output for optional pages, though we don't check the unfilled value of this answer anywhere IIRC. Someone who's been on the team longer than me can give a better answer here :) I don't feel very strongly about it either way, mostly just following convention.
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.
@DrewProebstel for more context
also @jenny-heath @embarnard @mpidcock @tahsinaislam if there was more reasoning for this choice (unfilled is similar to
nil
, 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.
FWIW I think that the decision to move away from using "unfilled" rather than nil as a default value in the app is well outside the scope of this PR, so discussion on that should probably happen elsewhere for posterity & visibility
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.
can we call this
has_credit_count_key
to be more clear