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

Subs: prevent fees from displaying in email when subscription order cannot be fulfilled #2122

Merged
merged 3 commits into from
Jun 6, 2018

Conversation

oeoeaio
Copy link
Contributor

@oeoeaio oeoeaio commented Mar 7, 2018

What? Why?

When none of the items requested in a subscription are available when an order cycle opens, the customer is sent a notification email to let them know that an order could not be placed. Despite the fact that the email says that the order was not processed, some fees were still showing in the order summary shown in the email. This PR should resolve that issue.

What should we test?

Notification emails for customers indicating a failure to place an order due to insufficient stock should not show fees of any sort.

Release notes

Feature is yet to be released so this does not require its own release notes.

@@ -147,8 +147,10 @@
allow(job).to receive(:unavailable_stock_lines_for) { order.line_items }
end

it "does not place the order, sends an empty_order email" do
it "does not place the order, clears, all adjustments, and sends an empty_order email" do
expect(order.adjustments).to_not be_empty
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we really need to add this one. Our only expectation is the one you added below. What happens before is not that relevant IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I'm happy to remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sauloperez please approve if you are happy

@oeoeaio oeoeaio force-pushed the subs-unplaced-fees branch from 9f7882b to eacf005 Compare March 8, 2018 00:51
@mkllnk
Copy link
Member

mkllnk commented Apr 10, 2018

Staged on https://staging1.openfood.com.au/.

@mkllnk
Copy link
Member

mkllnk commented Apr 10, 2018

@myriamboure I'm surprised this didn't get picked up for testing last night. Do I need to notify people? I'm working Tuesday to Friday now and the next two Mondays as well. I hope to stage something every working day. But do we have testers every day? I'm happy to continue this conversation in Slack.

@myriamboure
Copy link
Contributor

Oups @mkllnk actually @RachL and I are in Barcelona working with the Coopdevs team and spent the whole day on product chain inception, we will finish that tomorrow. We can try to do some testing in the day though to take advantage of you being able to stage every working day this week, we'll see but this week is a bit special because of colocating French and Catalunya team working on bigger picture inceptions first deep dive ;)

@sstead
Copy link

sstead commented Apr 11, 2018

Testing Notes

  • Fees aren't being listed, but the subtotal and total are not $0, which is what I'd expect. The total is including the value of enterprise fees.

https://docs.google.com/document/d/10AjV6FRGhsjLo4OtR3vQ8qKJrJPHnpTDVYtXi2ZSmbQ/edit#

@oeoeaio oeoeaio force-pushed the subs-unplaced-fees branch from 82f7e2f to 936cbcd Compare April 11, 2018 06:29
@myriamboure
Copy link
Contributor

Put that back in code review as it seems @oeoeaio already fixed the issue after Sally tested.

@mkllnk
Copy link
Member

mkllnk commented Apr 11, 2018

Ah, Rob had actually staged it already for testing. I just tried to put it back into testing after reviewing it, but my Zenhub stopped working. :-(

@mkllnk mkllnk force-pushed the subs-unplaced-fees branch from 936cbcd to 6a71aaf Compare April 16, 2018 07:07
@mkllnk
Copy link
Member

mkllnk commented Apr 16, 2018

@myriamboure This is staged on https://staging1.openfood.com.au/ again.

@myriamboure myriamboure self-assigned this Apr 17, 2018
@myriamboure
Copy link
Contributor

Testing notes: https://docs.google.com/document/d/10AjV6FRGhsjLo4OtR3vQ8qKJrJPHnpTDVYtXi2ZSmbQ/edit?usp=sharing

It seems the bug is solved BUT I have the impression that a new one has been introduced, I'm not sure if that's connected to this one or not. When I create a new subscription, in the "subscription dashboard" "items" column, the total is not consistent with subtotal and I'm not able to understand how it is calculated (see red lines in testing notes.) Can you have a look @oeoeaio or @sstead and tell if it's connected to that PR or if we need to open a new issue?

@myriamboure myriamboure removed their assignment Apr 17, 2018
@oeoeaio
Copy link
Contributor Author

oeoeaio commented Apr 20, 2018

HI @myriamboure,

Not sure about the issue you found. Definitely troubling that the different between the subtotal and the total is not itemised. I will attempt to replicate and drill down into the issue.

@oeoeaio
Copy link
Contributor Author

oeoeaio commented May 30, 2018

Ok, so the issue that @myriamboure picked up is unrelated to this PR, I have addressed it in #2346.

This can go through as long as no other issues are found.

@sauloperez
Copy link
Contributor

@myriamboure do you agree on merging this as per @oeoeaio's comment?

@myriamboure
Copy link
Contributor

I guess so @sauloperez I didn't retest but if the bug I found was not connected and is addressed somewhere else anyway this should be good to go!

@sauloperez sauloperez merged commit 7ff7fe4 into openfoodfoundation:master Jun 6, 2018
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.

8 participants