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 off-by-one error in fulfillment code #15

Merged
merged 4 commits into from
Apr 8, 2024
Merged

Conversation

tomwheeler
Copy link
Collaborator

What was changed

This fixes an off-by-one error (selection of an array slice omits the final element) in the Activity that creates a fulfillment.

Why?

When submitting an order for two items, I found that there were two fulfillments, as expected. However, I also found that the second one was empty.

Checklist

  1. Closes: N/A

  2. How was this tested:
    I tested this manually, observing that the second fulfillment was empty (before the change) and that it contained the expected content (after the change). I hope to create an automated test to validate the desired behavior, but wanted to submit a draft PR with the fix first.

  3. Any docs updates needed?
    No

@tomwheeler tomwheeler marked this pull request as ready for review April 4, 2024 22:14
order/activities_test.go Outdated Show resolved Hide resolved
order/activities_test.go Outdated Show resolved Hide resolved
@robholland
Copy link
Collaborator

For the no items case, the activity should cleanly return an empty result. That should be covered by a test. From a business point of view, the workflow should reject orders with no items, but the activity code should still handle that case.

@robholland
Copy link
Collaborator

Approved because I'm fine for the new edge case and test to be in a different PR if you prefer.

@tomwheeler
Copy link
Collaborator Author

I just pushed a couple of commits that simplify the two tests and handle the zero-items case (along with a test to verify the behavior).

@tomwheeler tomwheeler merged commit 6b67f8f into main Apr 8, 2024
2 checks passed
@tomwheeler tomwheeler deleted the fulfillment-off-by-one branch April 8, 2024 00:54
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.

3 participants