-
Notifications
You must be signed in to change notification settings - Fork 58
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
Add template code for multi-class discounts in rust #588
Conversation
cb28b1c
to
c3da6f0
Compare
name = "{{handle | replace: " ", "-" | downcase}}" | ||
version = "1.0.0" | ||
edition = "2021" | ||
rust-version = "1.62" |
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 version is old. We currentl use 1.83, should we update it? 1.62 is the one used in the other targets (older templates in this repo)
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 don't know for sure, but it seems sensible to start out with the latest Rust version that we're supporting in the CLI, rather than something older.
b8857ca
to
f8645e9
Compare
3e2f2b6
to
49535f4
Compare
query_path = "src/run.graphql", | ||
schema_path = "schema.graphql" | ||
)] | ||
fn cart_run(input: CartResponseData) -> Result<FunctionCartRunResult> { |
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 cart_run
will make:
- return no discount if the previous response was not a 200. (Could check specifically for 401, or the body)
- a X% order percent discount on subTotal target
- a Y% product discount on the highest cartLine cost (any better idea?)
query_path = "src/run.graphql", | ||
schema_path = "schema.graphql" | ||
)] | ||
fn delivery_run(input: DeliveryResponseData) -> Result<FunctionDeliveryRunResult> { |
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 delivery_run
will make:
- no discount if fetch did not return a 200. As above, I could check if 401 or the body.
- a Z% discount on all delivery groups
query_path = "src/fetch.graphql", | ||
schema_path = "schema.graphql" | ||
)] | ||
fn cart_fetch(input: CartFetchResponseData) -> Result<FunctionCartFetchResult> { |
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.
Fetch will return an HttpRequest passing down the email of the buyer. Another idea?
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 could have Fetch validate the enteredDiscountCodes and return them in the Fetch result. Then the Run result can include the addValidDiscountCodes
operation with the codes that Fetch said were valid.
Also, should the code for Fetch be in a different fetch.rs
file? I see that's what other Function templates have done (example).
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 could have Fetch validate the enteredDiscountCodes and return them in the Fetch result. Then the Run result can include the addValidDiscountCodes operation with the codes that Fetch said were valid.
Good idea. I will make that change.
Also, should the code for Fetch be in a different fetch.rs file? I see that's what other Function templates have done (example).
I have to say, I'm not sure about this. Can probably have different files. I'll have a look .
query_path = "src/fetch.graphql", | ||
schema_path = "schema.graphql" | ||
)] | ||
fn delivery_fetch(input: DeliveryFetchResponseData) -> Result<FunctionDeliveryFetchResult> { |
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.
Fetch will return an HttpRequest passing down the email of the buyer. Another idea?
49535f4
to
43d48f8
Compare
43d48f8
to
d246160
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.
I can't approve because I created this PR 😢 (which is deceptive because you've done all the work here!), but just to say that none of my comments are blocking and any that we want to address can be done iteratively in follow-up PRs.
[package] | ||
name = "{{handle | replace: " ", "-" | downcase}}" | ||
version = "1.0.0" | ||
edition = "2021" |
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.
Should this be "2025"?
name = "{{handle | replace: " ", "-" | downcase}}" | ||
version = "1.0.0" | ||
edition = "2021" | ||
rust-version = "1.62" |
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 don't know for sure, but it seems sensible to start out with the latest Rust version that we're supporting in the CLI, rather than something older.
You can build this individual function using `cargo build`. | ||
|
||
```shell | ||
cargo build --target=wasm32-wasip1 --release |
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.
Wasn't this target added with the newer Rust version? Seems this could also be a reason to bump the Rust version.
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.
That is true, however this target has been available before this release, it just became the new default with 1.84
buyerIdentity { | ||
} | ||
} |
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 should include enteredDiscountCodes
in the fetch input query.
query_path = "src/fetch.graphql", | ||
schema_path = "schema.graphql" | ||
)] | ||
fn cart_fetch(input: CartFetchResponseData) -> Result<FunctionCartFetchResult> { |
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 could have Fetch validate the enteredDiscountCodes and return them in the Fetch result. Then the Run result can include the addValidDiscountCodes
operation with the codes that Fetch said were valid.
Also, should the code for Fetch be in a different fetch.rs
file? I see that's what other Function templates have done (example).
id: cart_line_id.to_string(), | ||
quantity: Some(1), | ||
})], | ||
associated_discount_code: None, |
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.
If we add enteredDiscountCode validation in the Fetch function, then we could include an associated_discount_code from the valid discount codes returned by the Fetch.
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.
Let's merge this and improve iteratively as suggested by Linda. 🙌
@lindadamus I've updated the code. Lmk if that works with you. In the meantime, I'll merge it and please comment here and I will fast follow on it. |
45323b5
to
a258837
Compare
Looks good! I haven't done a tophat but should be able to tomorrow when all the pieces are in place (CLI, partners, templas). |
This creates three discounts: