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

Fix: update Product.item components #64

Merged
merged 5 commits into from
Jun 23, 2024

Conversation

madcalf
Copy link
Contributor

@madcalf madcalf commented Jun 10, 2024

Updates the Product.info components' field names to be more consistent and/or clarify their intent.
Also removes the default values in the weight component.

Closes #63

@madcalf madcalf requested a review from jtfairbank June 10, 2024 04:57
Copy link
Collaborator

@jtfairbank jtfairbank left a comment

Choose a reason for hiding this comment

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

  • Praise: Great job touching up all of the documented consistency issues!
  • Suggestion (blocking): Noticed a few others while I was reviewing. Can we address them in this PR too?
    • for item.needsMet, rename monthlyNeedsMetPerUnit to monthlyNeedsMetPerItem so that we're using the item / unit terminology consistently
    • for item.value make the source and logDate fields required, to be consistent with item.volume and item.weight
  • Question: For item.value, do you think it'd be worth adding a packagePriceUnit field, set to USD as a constant for now? And what about naming itemPrice to itemPriceUSD for consistency with itemVolume and itemWeight which include units in the field name?

@jtfairbank
Copy link
Collaborator

@madcalf testing 123

@madcalf madcalf closed this Jun 19, 2024
@madcalf madcalf reopened this Jun 19, 2024
@madcalf
Copy link
Contributor Author

madcalf commented Jun 19, 2024

rename monthlyNeedsMetPerUnit to monthlyNeedsMetPerItem so that we're using the item / unit terminology consistently

Rename is done. Will make monthlyNeedsMetPerItem a calculated field in a separate PR.

@madcalf
Copy link
Contributor Author

madcalf commented Jun 19, 2024

For item.value, do you think it'd be worth adding a packagePriceUnit field, set to USD as a constant for now?

Sure that makes sense. Added it as an enum with USD as the default (and currently the only) option, and we can add in additional currency values needed.

@madcalf
Copy link
Contributor Author

madcalf commented Jun 19, 2024

And what about naming itemPrice to itemPriceUSD for consistency with itemVolume and itemWeight which include units in the field name?

Renamed itemPrice to pricePerItemUSD because i kept getting it confused with the packagePrice and it seems more in line with the other calculated field names. I'll also make this one a calculated field in a separate PR.

@madcalf madcalf requested a review from jtfairbank June 19, 2024 23:07
@jtfairbank jtfairbank force-pushed the fix/update-productitem-components branch from 1da3560 to b3e3e96 Compare June 23, 2024 11:38
@jtfairbank jtfairbank merged commit 5a85f43 into saga Jun 23, 2024
1 check passed
@jtfairbank jtfairbank deleted the fix/update-productitem-components branch June 23, 2024 11:40
@jtfairbank
Copy link
Collaborator

@madcalf looks great!

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.

Update field names in Product.item components
2 participants