[City OFN Voucher] A shopper can use a VINE voucher #12949

rioug · 2024-10-29T00:44:52Z

⚠️ When working on this use the Clockify code: Citi Food Vouchers: #1A. #11922 Citi OFN Voucher - VINE Integration ⚠️

What? Why?

Closes [Citi OFN Vouchers] As a shopper I can use a voucher created with VINE #12859

It lets a customer use a VINE voucher on their order. OFN will communicate with the VINE Api :

First, to validate a voucher when it's added to the order
Then on checkout, to redeem the voucher
VINE voucher are managed on the VINE platform : https://vine-staging.openfoodnetwork.org.au/login , as such they won't show on the voucher admin page.

What should we test?

Set up:
!! Make sure "connected_apps" feature is turned on
Logged as an enterprise to VINE, in the backoffice as an enterprise manager :

Visit the enterprise setting and click on "connected apps" in the left hand side menu
Enter you api key and associated secret in the "Voucher Integration Engine (VINE)" section

Log into VINE https://vine-staging.openfoodnetwork.org.au/ :

set your team to "OFN test - service" by clicking on your name in the top right hand side choosing "OFN test - service"
Click on "Voucher sets" on the top right hand side menu, you should now see a voucher set ( NOTE: the two last voucher set have $1 voucher, I would recommend using those ones for testing, unless testing partial redemption/redemption by multiple enterprises)
Clicking on the voucher set, you'll get access to a list of voucher you can use for testing

Normal checkout

Make sure you have an order cycle set up with the enterprise you connected to VINE set as a distributor, set your team in VINE to "OFN test - merchant"

start an order and proceed to checkout
advance to payment method
--> you should see a voucher input
- try adding a random code
  --> you should get a not found error
- try adding a voucher using the short code
  --> the 1$ voucher should be added to the order
- switch to a different team in VINE, and try adding a voucher using the short code
  --> you should get There was an error while adding the voucher
with a voucher added to the order, proceed to order summary
--> you should see the voucher applied to the order in the right hand side
Complete the order
--> you should see the voucher on the order confirmation page
--> you should see the voucher in the confirmation email
Login in VINE, set your team to "OFN test - service",
Navigate to the voucher you used
--> you should see "0$ Remaining value" and " 1 # Redemptions"

Same voucher used by multiple enterprise

Same as above, but this time use a $50 voucher from the first voucher set and place order under $50 so we can use the voucher with another enterprise.

Connect a different enterprise to VINE, you can use the same credentials for multiple enterprises.
Make sure you have an order cycle set up with the enterprise you just connected to VINE
Set your team in VINE to "OFN test - merchant 2"
Place an order with the same voucher you used above
--> you should see the voucher on the order confirmation page
--> you should see the voucher in the confirmation email
Login in VINE, set your team to "OFN test - service",
Navigate to the voucher you used
--> you should see " 2 # Redemptions" and under the "Voucher Redemptions" section, you should see a redemption for each "OFN test - merchant" and OFN test - mechant 2"

Failed redemption :

same as before, start an order, proceed to checkout, add a voucher and proceed to "Order summary"
Login in VINE and switch your team "OFN test - service"
Complete your order
--> you should get "Redeeming the voucher failed" error

Admin - capture a payment

same as before , start an order, proceed to checkout, add a voucher and proceed to "Order summary"
go to the backoffice Orders screen
Select the order you just created, it should be in the "confirmation" state
--> you should see the voucher as an "Order Adjustments"
Click on "payments" on the right hand side menu
Capture and complete the order by clicking on the ✅ button
--> The payment status should be "Complete" and order status on the right hand side should also be "Compete"

Failed redemption:

Same as before up to the payment screen
Capture and complete the order by clicking on the ✅ button
--> you should get "Redeeming the voucher failed" error

Admin - create a payment

same as before , start an order, proceed to checkout, add a voucher and proceed to "Order summary"
go to the backoffice Orders screen
Select the order you just created, it should be in the "confirmation" state
Click on "payments" on the right hand side menu
Click on "+ New Payment" button
Enter details and click on "Update"
--> you should get redirected to the payment page and the new payment should be listed, order status should be "complete"

Failed redemption:

Same as before up to the new payment screen
Enter details and click on "Update"
--> should get redirected to the payment page and get "Redeeming the voucher failed" error

Report

Report will be available via Metabase and/or VINE.
~~Check the voucher are showing up correctly on the following reports:~~
~~- Order Cycle Customer Totals~~
~~- Enterprise Fees With Tax Report By Order~~
~~- Revenues By Hub~~
~~- Sales Tax Totals By Order~~

Release notes

Changelog Category (reviewers may add a label for the release notes):

User facing changes
API changes (V0, V1, DFC or Webhook)
Technical changes only
Feature toggled

The title of the pull request will be included in the release notes.

rioug · 2024-10-29T05:28:41Z

app/controllers/voucher_adjustments_controller.rb

    render cable_ready: cable_car.replace(
      selector: "#checkout-payment-methods",


I didn't replace this by turbo, as it will be done in this PR #12896

dacook

Partial review, up to and including "Add VineVoucherValidatorService and spec"

spec/services/vine_api_service_spec.rb

config/locales/en.yml

app/services/vine_voucher_validator_service.rb

app/models/voucher.rb

spec/services/vine_voucher_validator_service_spec.rb

dacook

Forgot to include a couple of thoughts on the data model:

...e/20241008053852_add_external_voucher_id_external_voucher_set_id_voucher_type_to_vouchers.rb

dacook

Also reviewed "Handle adding a VINE voucher to an order".
I will pause until we can discuss next week.

app/controllers/voucher_adjustments_controller.rb

dacook

Big effort! 💪

I have lots of comments sorry. I think because my brain is averse to complexity and tries to find simpler ways. I think you put it well when you said:

we are trying to shoehorn VINE voucher into a model that wasn't really design for that, on top of voucher already been shoehorned into adjustments

So I'm cautious about creating tech debt that we will have to pay for in the future.

spec/system/consumer/checkout/payment_spec.rb

dacook · 2024-11-06T03:52:59Z

spec/system/consumer/checkout/payment_spec.rb

+                  fill_in "Enter voucher code", with: "KM1891"
+                  click_button("Apply")
+
+                  expect(page).to have_content("There was an error while adding the voucher")


Are we able to say why there was an error? Does the invalid_voucher or not_found_voucher message appear here?

app/services/vine/voucher_validator_service.rb

app/services/vine/api_service.rb

app/services/vine_voucher_redeemer_service.rb

dacook · 2024-11-06T05:25:01Z

app/controllers/checkout_controller.rb

+      flash[:error] = if vine_voucher_redeemer.errors.keys.include?(:redeeming_failed)
+                        vine_voucher_redeemer.errors[:redeeming_failed]
+                      else
+                        I18n.t('checkout.errors.voucher_redeeming_error')
+                      end


Can't we rely on vine_voucher_redeemer to give us the error we need to know about? IE:

Suggested change

flash[:error] = if vine_voucher_redeemer.errors.keys.include?(:redeeming_failed)

vine_voucher_redeemer.errors[:redeeming_failed]

else

I18n.t('checkout.errors.voucher_redeeming_error')

end

flash[:error] = vine_voucher_redeemer.errors.first

Or if not, maybe we can use optional chaining like so:

Suggested change

flash[:error] = if vine_voucher_redeemer.errors.keys.include?(:redeeming_failed)

vine_voucher_redeemer.errors[:redeeming_failed]

else

I18n.t('checkout.errors.voucher_redeeming_error')

end

flash[:error] = vine_voucher_redeemer.errors&[:redeeming_failed] ||

I18n.t('checkout.errors.voucher_redeeming_error')

spec/requests/spree/admin/payments_spec.rb

app/controllers/spree/admin/payments_controller.rb

rioug · 2024-11-13T05:21:26Z

I am yet to address all the review comments, but I had to do a bit more work to handle this requirement:

a voucher to be able to be redeemed by multiple merchants

I ended up creating Vouchers::Vine because in the end it is different enough from Vouchers::FlatRate to have its' own class, mainly because we now have different requirement on the uniqueness of a voucher code.

@dacook I started looking into handling API error with exception, and you are right it should be a bit cleaner than the current solution.

Moving back into code review because I think it's worth Maikel having a look even if it's not totally finished

dacook

Great, thanks for trying that, I think it looks simpler now. What do you think?

mkllnk

Great work, @rioug!

And sorry that it took me so long to review. There are some minor style issues we could address but I don't really want to add to this epic pull request. We can work on it in follow-up pull requests.

The only big questions for me now are:

Is the data model right?
Are we doing the correct validations?

Adding the external vine attributes is definitely not ideal. And I think that we could get around it by calling the validation API before redemption. That's maybe how it was intended. But those fields are also not in the way of anything. We could get rid of them quite easily in the future. So no need to act right now.

Those fields are also used in a find method. And just for my own clarification, this is how I read the code:

User enters code.
We can't find it locally and ask Vine.
We get the set id etc and store the voucher locally.
This info is used for redemption at checkout.
The same voucher could be used again.
Again, we don't find it locally and ask Vine.
We get the same ids back and find it locally to update.
Now the voucher value changed. If we ran reports about redemptions in OFN, we would only see the last remaining value of the voucher and not its initial value. Potential issue later?

Just thinking about alternatives:

We don't need to store the short code because we never use it for redemption, just to display to the user.
We could use "#{voucher_set_id}/#{voucher_id}" as local voucher id which would always be unique per enterprise. We wouldn't need to soften the validations.
Is the short code part of the long id? Can we derive it or do we need to store it separately?

Downside of the above would be that we need to change all the views to not display a really long voucher id.

Should we always create a new voucher instead of finding a previous use of the same code?

That could avoid race conditions if the same code is used by two people. But that's unlikely. We would be able to track the value at the time of each redemption though.

What do you think?

mkllnk · 2024-11-20T01:13:29Z

app/controllers/voucher_adjustments_controller.rb

+    voucher = Voucher.find_by(code: voucher_params[:voucher_code],
+                              enterprise: @order.distributor)
+    return voucher unless voucher.nil? || voucher.is_a?(Vouchers::Vine)
+
+    vine_voucher


No need to change now but generally I find this pattern more flexible:

local_voucher || vine_voucher

Then you can implement the parts on their own and test them independently.

app/controllers/checkout_controller.rb

app/services/vine/api_service.rb

app/validators/vouchers/scoped_uniqueness_validator.rb

It is used to validate a voucher using the given short code

A Vine voucher is really a specific type of FlatRate voucher but because a Vine voucher can be used by mutiple enterprise, it can be considered different enough to warrant it's own class. It still share a lot of the behaviour of a FlatRate voucher, so to avoid duplication, all the shared functionality have been moved to a Vouchers::FlatRatable concern.

Plus optimise code with `find_or_initialize_by` as suggested in review

Paranoia doesn't support unique validation including deleted records: rubysherpas/paranoia#333 We use a custom validator, ScopedUniquenessValidator to avoid the issue

Log Client and Server error, and re raise exception for the caller to handle

RachL · 2024-11-28T12:59:06Z

@rioug thanks for the detailed testing notes, it's really helpfull!

Main testing scenario passed successfully 💪 A few screenshots I might reuse later:

however for the reports sections, only one report mentioned displayed vouchers

Order Cycle Customer Totals ✔️
Enterprise Fees With Tax Report By Order 🚫
Revenues By Hub 🚫
Sales Tax Totals By Order 🚫

Is this list still accurate? I'm surprised to see the revenues by hub here given it's a report that is only accessible to super admins.

I'm ok to merge this as is and focus on reports in another issue, what do you think @rioug ? there are failing check also to consider before merging.

I also noticed something I've missed in my preview testing:

the sentence above the link should not appear once the API key is settled. Also the link does not redirect to VINE. I've opened this issue about it: #13006

RachL · 2024-11-28T20:03:45Z

@rioug I also have a question about the test scenario: why do we need to "Set your team in VINE to "OFN test - merchant 2" ?

I know staying login as OFN - test service will throw an error at checkout, but staying logout of VINE works as well (and that's correct as the shopper won't have a VINE account).
Do you remember why switching to the merchant account was important?

rioug · 2024-12-02T00:46:59Z

Now the voucher value changed. If we ran reports about redemptions in OFN, we would only see the last remaining value of the voucher and not its initial value. Potential issue later?

That could be an issue indeed, that said I would expect VINE related report to be handled in VINE itself, or metabase. We need to update the value here as we need it in OFN to calculate the voucher adjustment amount ie :

https://github.com/openfoodfoundation/openfoodnetwork/pull/12949/files#diff-e239f12abad049acb9fbe978f1c7aff5303f2bd384d766a98c688678d64c3ae7R21-R23

If we keep the original value, we could potentially try to redeem more than what's available and the customer won't be able to place the order.

Just thinking about alternatives:

We don't need to store the short code because we never use it for redemption, just to display to the user.

I still think it's good to store it, it might be useful for debugging purposes I'd assume customer will only have access to the short code.

We could use "#{voucher_set_id}/#{voucher_id}" as local voucher id which would always be unique per enterprise. We wouldn't need to soften the validations.

Is the short code part of the long id? Can we derive it or do we need to store it separately?

I am not sure how the short code is generated, as far as I can tell it's not part of the long id. We could have a look at the VINE implementation, but I don't think it's a good idea to copy the functionality.

It's true we don't actually need the short code from a technical stand point, but we need it for customer feedback, ie we need to be able to display the code used to the customer on the checkout screen and order confirmation email.

Should we always create a new voucher instead of finding a previous use of the same code?

That could avoid race conditions if the same code is used by two people. But that's unlikely. We would be able to track the value at the time of each redemption though.

I guess we could but wouldn't we have the same issue you raised above if we were to run reports about redemption in OFN ? We also would not have a unique voucher_set_id/voucher_id per enterprise any more.

My understanding is for VINE to cover functionalities originally included in phase 2 of vouchers, so I am not expecting to see redemption report in OFN. But still it's good to keep this in mind.

@mkllnk hopefully this answers your questions. I don't think we need to action anything for now.

rioug · 2024-12-02T00:56:36Z

@RachL for voucher in report see : #11763
That said my understanding is voucher related report will be available through VINE/Metabase.

rioug · 2024-12-02T01:00:17Z

@rioug I also have a question about the test scenario: why do we need to "Set your team in VINE to "OFN test - merchant 2" ?

I know staying login as OFN - test service will throw an error at checkout, but staying logout of VINE works as well (and that's correct as the shopper won't have a VINE account). Do you remember why switching to the merchant account was important?

This to test a voucher can actually be redeemed by multiple enterprise. The test note don't exactly reflect a real life scenario, in reality each enterprise would be using their own set of credentials and would be linked to their own team in VINE. To make testing easier and not having to set up another set of credentials, you can just switch your team to a different Merchant to simulate that.

RachL · 2024-12-02T16:03:37Z

@rioug yes that was my understanding as well for reports, I was confused by the reports section of the testing section of the PR. I think we can remove it?

In that case and if Maikel's comments are all addressed, it looks like we can merge? I won't be at delivery circle tomorrow (that's the time slot I can't make) but feel free to merge if we are all aligned. Kudos :)

github-actions bot assigned rioug Oct 29, 2024

rioug force-pushed the 12859-use-VINE-voucher branch 2 times, most recently from 84c1dfd to 0a0ae42 Compare October 29, 2024 01:08

rioug changed the title ~~[City OFN Voucher] Use vine voucher~~ [City OFN Voucher] A shopper can use a VINE voucher Oct 29, 2024

rioug marked this pull request as ready for review October 29, 2024 05:20

rioug commented Oct 29, 2024

View reviewed changes

dacook self-requested a review October 30, 2024 01:15

dacook reviewed Oct 30, 2024

View reviewed changes

...e/20241008053852_add_external_voucher_id_external_voucher_set_id_voucher_type_to_vouchers.rb Show resolved Hide resolved

...e/20241008053852_add_external_voucher_id_external_voucher_set_id_voucher_type_to_vouchers.rb Show resolved Hide resolved

dacook reviewed Oct 31, 2024

View reviewed changes

app/controllers/voucher_adjustments_controller.rb Outdated Show resolved Hide resolved

app/controllers/voucher_adjustments_controller.rb Outdated Show resolved Hide resolved

rioug force-pushed the 12859-use-VINE-voucher branch from 572271e to 544dcfc Compare November 4, 2024 05:25

dacook self-requested a review November 6, 2024 03:29

dacook reviewed Nov 6, 2024

View reviewed changes

rioug force-pushed the 12859-use-VINE-voucher branch 3 times, most recently from 9907b82 to d1be702 Compare November 13, 2024 03:41

RachL added pr-staged-au staging.openfoodnetwork.org.au and removed pr-staged-au staging.openfoodnetwork.org.au labels Nov 15, 2024

dacook approved these changes Nov 18, 2024

View reviewed changes

rioug added pr-staged-au staging.openfoodnetwork.org.au feature toggled These pull requests' changes are invisible by default and are grouped in release notes and removed pr-staged-au staging.openfoodnetwork.org.au labels Nov 19, 2024

mkllnk approved these changes Nov 20, 2024

View reviewed changes

mkllnk force-pushed the 12859-use-VINE-voucher branch from 063ff57 to 5852708 Compare November 26, 2024 01:02

RachL added pr-staged-au staging.openfoodnetwork.org.au and removed pr-staged-au staging.openfoodnetwork.org.au labels Nov 28, 2024

RachL self-assigned this Nov 28, 2024

VineApiService, add voucher_validation

7cbe776

It is used to validate a voucher using the given short code

rioug added 10 commits November 28, 2024 13:35

Add voucher used by multiple enterprise and recycle code scenario

48ad7ed

Plus optimise code with `find_or_initialize_by` as suggested in review

Update various voucher related file to use the new Vouchers::Vine

d413a14

Per review, fix a some minor issues

9ab2a3a

Fix unique validator for vouche code

73819a4

Paranoia doesn't support unique validation including deleted records: rubysherpas/paranoia#333 We use a custom validator, ScopedUniquenessValidator to avoid the issue

Re worked the Vine::ApiService to raise exception on error

1b50217

Log Client and Server error, and re raise exception for the caller to handle

Fix Vine::VoucherValidatorService to handle exceptions

d102652

Fix Vine::VoucherRedeemerService to handle exceptions

1e6fbad

Remove unused error translation

16d6e1f

Use the voucher adjustment amount for redeeming

f5b9ca3

RachL force-pushed the 12859-use-VINE-voucher branch from 5852708 to f5b9ca3 Compare November 28, 2024 12:35

RachL added pr-staged-au staging.openfoodnetwork.org.au and removed pr-staged-au staging.openfoodnetwork.org.au labels Nov 28, 2024

filipefurtad0 added pr-staged-uk staging.openfoodnetwork.org.uk and removed pr-staged-uk staging.openfoodnetwork.org.uk labels Nov 29, 2024

RachL removed the pr-staged-fr staging.coopcircuits.fr label Nov 29, 2024

dacook approved these changes Dec 2, 2024

View reviewed changes

rioug removed the feedback-needed label Dec 2, 2024

mkllnk merged commit de938f6 into openfoodfoundation:master Dec 3, 2024
60 of 61 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[City OFN Voucher] A shopper can use a VINE voucher #12949

[City OFN Voucher] A shopper can use a VINE voucher #12949

rioug commented Oct 29, 2024 •

edited

Loading

rioug Oct 29, 2024

dacook left a comment

dacook left a comment

dacook left a comment •

edited

Loading

dacook left a comment

dacook Nov 6, 2024

dacook Nov 6, 2024

rioug commented Nov 13, 2024

dacook left a comment

mkllnk left a comment

mkllnk Nov 20, 2024

RachL commented Nov 28, 2024 •

edited

Loading

RachL commented Nov 28, 2024

rioug commented Dec 2, 2024

rioug commented Dec 2, 2024

rioug commented Dec 2, 2024

RachL commented Dec 2, 2024

		render cable_ready: cable_car.replace(
		selector: "#checkout-payment-methods",

[City OFN Voucher] A shopper can use a VINE voucher #12949

[City OFN Voucher] A shopper can use a VINE voucher #12949

Conversation

rioug commented Oct 29, 2024 • edited Loading

What? Why?

What should we test?

Normal checkout

Same voucher used by multiple enterprise

Failed redemption :

Admin - capture a payment

Failed redemption:

Admin - create a payment

Failed redemption:

Report

Release notes

rioug Oct 29, 2024

Choose a reason for hiding this comment

dacook left a comment

Choose a reason for hiding this comment

dacook left a comment

Choose a reason for hiding this comment

dacook left a comment • edited Loading

Choose a reason for hiding this comment

dacook left a comment

Choose a reason for hiding this comment

dacook Nov 6, 2024

Choose a reason for hiding this comment

dacook Nov 6, 2024

Choose a reason for hiding this comment

rioug commented Nov 13, 2024

dacook left a comment

Choose a reason for hiding this comment

mkllnk left a comment

Choose a reason for hiding this comment

mkllnk Nov 20, 2024

Choose a reason for hiding this comment

RachL commented Nov 28, 2024 • edited Loading

RachL commented Nov 28, 2024

rioug commented Dec 2, 2024

rioug commented Dec 2, 2024

rioug commented Dec 2, 2024

RachL commented Dec 2, 2024

rioug commented Oct 29, 2024 •

edited

Loading

dacook left a comment •

edited

Loading

RachL commented Nov 28, 2024 •

edited

Loading