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

[Bug] Level 1 - Order validation shouldn't depend on ordering of items #53

Closed
evarga opened this issue Dec 8, 2023 · 2 comments
Closed
Labels
bug Something isn't working

Comments

@evarga
Copy link
Contributor

evarga commented Dec 8, 2023

Summary

The provided solution.py wrongly implements order validation. Currently, the outcome depends on how items are arranged in an order.

How to reproduce

Just add the following test case to the tests.py file and change line number 2 to import solution instead of code. Order 1 will be properly detected as exceeding the allowed maximum amount, but Order 2 will be wrongly accepted.

# Example 4 - order validation shouldn't depend on ordering of items
def test_4(self):
    num_items = 12
    items = [c.Item(type='product', description='tv', amount=99999, quantity=num_items)]
    for i in range(num_items):
        items.append(c.Item(type='payment', description='invoice_' + str(i), amount=99999, quantity=1))
    order_1 = c.Order(id='1', items=items)
    self.assertEqual(c.validorder(order_1), 'Total amount exceeded')

    # Put payments before products
    items = items[1:] + [items[0]]
    order_2 = c.Order(id='2', items=items)
    self.assertEqual(c.validorder(order_2), 'Total amount exceeded')

How to Fix this Bug

There are couple of possibilities as enrolled below:

  1. Use separate variables for total payments and expenses; use the latter for checking whether the total maximum amount is exceeded or not.
  2. Handle products before payments.
@evarga evarga added the bug Something isn't working label Dec 8, 2023
@jkcso
Copy link
Collaborator

jkcso commented Dec 30, 2023

Thanks for pointing out through this amazing explanation! The goal was to show a bug that many people might not be aware about, so the proposed fixes are good to haves too, but they were not the initial priority. Please feel free to submit a PR that closes this issue so learners can learn even more :-)

@evarga evarga mentioned this issue Dec 31, 2023
2 tasks
jkcso pushed a commit that referenced this issue Jan 5, 2024
@jkcso
Copy link
Collaborator

jkcso commented Jan 5, 2024

Fixed, thank you!

@jkcso jkcso closed this as completed Jan 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants