fix: #202 - missing checks during matching for number

[JonBoyleCoding]

8ccfee5 Changed the matching algorithm, and did not put in a check on the match_amount as had previously been done in _get_weight_matches.

This fix adds the check to ensure that the number is within the fuzzy range.

[Zburatorul]

Good find. Could you add a unit test that would have caught the absence of this check?

[ankurdave]

@JonBoyleCoding Thanks for the fix! Here's a unit test that previously failed and now passes:

# In matching_test.py:

def test_nonmatch_fuzzy_amount_with_dates():
  # We don't match with a skew of 0.02, beyond our configured fuzzy_match_amount.
  assert_match(
      pending_candidate="""
      2023-01-01 * "Transaction 0"
        Assets:A 20.00 USD
          date: 2023-01-01
          cleared: TRUE
        Expenses:FIXME -20.00 USD
      """,
      pending="""
      2023-01-01 * "Transaction 1"
        Assets:B -19.98 USD
          date: 2023-01-01
          cleared: TRUE
        Expenses:FIXME 19.98 USD
      """)

[ankurdave]

It seems like this is because of the indexing logic here: 8ccfee5#diff-1d4595bc7bf509394ca45262f4656c4b180c83a7be5e6cb653b7265d29743c97R445

We currently modify the lower bound of the loop to include an element less than the lower bound:

for sp in cur_matches[lower_bound-1 if lower_bound > 0 else 0:upper_bound]:
    # Verify that number is in fuzzy range.
    # Currently fails (issue #202).
    assert abs(sp.number - amount.number) <= self.fuzzy_match_amount

Removing that fixes the problem:

for sp in cur_matches[lower_bound:upper_bound]:
    # Verify that number is in fuzzy range.
    # No longer fails.
    assert abs(sp.number - amount.number) <= self.fuzzy_match_amount

@xentac Do you remember why we modify lower_bound here?

[ankurdave]

Opened #210 with the above test and fix.

[JonBoyleCoding]

Sorry for not getting back with a test - I was partially unsure of how this should have been done and then work got in the way.

With the other above PRs, will this fix still be necessary and worth merging in? Or will they supercede the problem?

[ankurdave]

@JonBoyleCoding Not at all! It did take me a little while to figure out the test, even with the repro in #202. It turned out that the specific amounts were necessary to reproduce the bug.

#210 will supersede this PR.

[JonBoyleCoding]

Closing as #210 is now merged and supercedes.