Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 4.1.0-b2
    • Fix Version/s: 4.1.1, 4.2.0-b1
    • Component/s: None
    • Labels:
      None

      Description

      Invoices::getTotal() will not properly cascade if the level 2 tax is processed before the level 1 tax.

        Activity

        jonathan Jonathan Reissmueller created issue -
        jonathan Jonathan Reissmueller made changes -
        Field Original Value New Value
        Rank Ranked higher
        jonathan Jonathan Reissmueller made changes -
        Sprint 4.2.0 Sprint 1 [ 44 ]
        jonathan Jonathan Reissmueller made changes -
        Rank Ranked lower
        jonathan Jonathan Reissmueller made changes -
        Rank Ranked higher
        Automated transition triggered when Jonathan Reissmueller created a branch in Stash -
        Status Open [ 1 ] In Progress [ 3 ]
        tyson Tyson Phillips (Inactive) made changes -
        Rank Ranked lower
        tyson Tyson Phillips (Inactive) made changes -
        Sprint 4.2.0 Sprint 1 [ 44 ]
        tyson Tyson Phillips (Inactive) made changes -
        Rank Ranked higher
        Hide
        tyson Tyson Phillips (Inactive) added a comment -

        Could use more details on what this means exactly

        Show
        tyson Tyson Phillips (Inactive) added a comment - Could use more details on what this means exactly
        tyson Tyson Phillips (Inactive) made changes -
        Fix Version/s 4.1.1 [ 11015 ]
        tyson Tyson Phillips (Inactive) made changes -
        Sprint 4.2.0 Sprint 1 [ 44 ]
        tyson Tyson Phillips (Inactive) made changes -
        Rank Ranked lower
        Hide
        jonathan Jonathan Reissmueller added a comment - - edited

        We only actually perform a cascade using the level two tax. So 'level_2_tax_amount' = (price + level_1_tax_amount) * level_2_tax_rate, whereas level_1_tax_amount = price * level_1_tax_rate. If level_2_tax_amount is calculated before level_1_tax_amount it will substitute 0 so level_2_tax_amount = (price + 0) * level_2_tax_rate. Not really sure how to clarify this. The calculation for level two thinks there is no tax for level one if level one has not been calculated yet. Just a matter of fetching the tax rules from the database in the right order.

        Show
        jonathan Jonathan Reissmueller added a comment - - edited We only actually perform a cascade using the level two tax. So 'level_2_tax_amount' = (price + level_1_tax_amount) * level_2_tax_rate, whereas level_1_tax_amount = price * level_1_tax_rate. If level_2_tax_amount is calculated before level_1_tax_amount it will substitute 0 so level_2_tax_amount = (price + 0) * level_2_tax_rate. Not really sure how to clarify this. The calculation for level two thinks there is no tax for level one if level one has not been calculated yet. Just a matter of fetching the tax rules from the database in the right order.
        Hide
        tyson Tyson Phillips (Inactive) added a comment -

        It sounds like there is an issue with the calculation order then. Have you looked at the pricing presenter to see how it handles cascading taxes?

        price = sum(items)
        level1_amount = price * level1_rate
        level2_amount = (price + level1_amount) * level2_rate
        total = price + level1_amount + level2_amount
        
        Show
        tyson Tyson Phillips (Inactive) added a comment - It sounds like there is an issue with the calculation order then. Have you looked at the pricing presenter to see how it handles cascading taxes? price = sum(items) level1_amount = price * level1_rate level2_amount = (price + level1_amount) * level2_rate total = price + level1_amount + level2_amount
        Hide
        jonathan Jonathan Reissmueller added a comment -

        Have you looked at the function I mention in the task? It uses one query to fetch both level 1 and level 2 tax rules, then loops through them, calculating the amount for each. No I didn't look at the pricing presenter, but it must of course follow the progression you list. When the taxes are retrieved in the right order getTotal() does it in this form:

        price = sum(items)
        level1_amount = price * level1_rate
        tax_total = level1_amount 
        level2_amount = (price + tax_total ) * level2_rate
        tax_total += level2_amount 
        total = price + tax_total
        

        This is a super simple fix. Just needed to add an order by to make such the rules are fetched in the right order.

        Show
        jonathan Jonathan Reissmueller added a comment - Have you looked at the function I mention in the task? It uses one query to fetch both level 1 and level 2 tax rules, then loops through them, calculating the amount for each. No I didn't look at the pricing presenter, but it must of course follow the progression you list. When the taxes are retrieved in the right order getTotal() does it in this form: price = sum(items) level1_amount = price * level1_rate tax_total = level1_amount level2_amount = (price + tax_total ) * level2_rate tax_total += level2_amount total = price + tax_total This is a super simple fix. Just needed to add an order by to make such the rules are fetched in the right order.
        Hide
        tyson Tyson Phillips (Inactive) added a comment -

        You should create tests that show this problem not working correctly and your fix resolving it. It would be useful to have unit tests for this method if there aren't any already so we can continue to ensure it works correctly.

        Show
        tyson Tyson Phillips (Inactive) added a comment - You should create tests that show this problem not working correctly and your fix resolving it. It would be useful to have unit tests for this method if there aren't any already so we can continue to ensure it works correctly.
        Automated transition triggered when Jonathan Reissmueller created pull request #308 in Stash -
        Status In Progress [ 3 ] In Review [ 5 ]
        Resolution Fixed [ 1 ]
        jonathan Jonathan Reissmueller made changes -
        Remaining Estimate 0 minutes [ 0 ]
        Time Spent 1 hour, 23 minutes [ 4980 ]
        Worklog Id 10246 [ 10246 ]
        Automated transition triggered when Tyson Phillips (Inactive) merged pull request #308 in Stash -
        Status In Review [ 5 ] Closed [ 6 ]

          People

          • Assignee:
            jonathan Jonathan Reissmueller
            Reporter:
            jonathan Jonathan Reissmueller
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:
              Fix Release Date:
              27/Sep/17

              Time Tracking

              Estimated:
              Original Estimate - Not Specified
              Not Specified
              Remaining:
              Remaining Estimate - 0 minutes
              0m
              Logged:
              Time Spent - 1 hour, 23 minutes
              1h 23m

                Agile