Skip to content

[FIX] Cash Deposit Match #83

Merged
chelsea-EYDS merged 7 commits intomainfrom
cash
Apr 12, 2023
Merged

[FIX] Cash Deposit Match #83
chelsea-EYDS merged 7 commits intomainfrom
cash

Conversation

@chelsea-EYDS
Copy link
Contributor

@chelsea-EYDS chelsea-EYDS commented Apr 5, 2023

FIX

Objective:

  • bugfix
  • bug description: there were a few cases where cash deposits were being set to Match, but the id was not found in the payment table. Any deposit which is considered to be matched must have a corresponding (group) of payments. This bug was only occurring in cases with duplicate amounts/dates - the cause was in the matching logic as the original array was not being altered properly to indicate a match, and so the deposits would match, the id would be recorded on the corresponding payment, and then it would be overridden (but the original cash deposit was still recorded as a match)
  • queries to test:
select
	COUNT(distinct cash_deposit_match)
from
	payment p
where
	cash_deposit_match is not null
select
	COUNT(distinct id)
from
	cash_deposit
where
	status = 'MATCH'

The results of these queries should always be equal

  • In addition to the bugfix I have removed (some) of the Promise.all() methods where are they are not required.
  • I have also changed the functions for updating the status' during reconciliation to use the update() method rather than querying for the entity and saving. (Since we have the ids and we know they will exist it is seems like a better option and speeds up reconciliation a bit which is helpful for testing :)

@chelsea-EYDS chelsea-EYDS requested a review from vyasworks April 5, 2023 20:42
Copy link
Contributor

@vyasworks vyasworks left a comment

Choose a reason for hiding this comment

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

Looks good, before approving,

Now that we have test cases base setup, can we include a test case for this scenario?

vyasworks
vyasworks previously approved these changes Apr 11, 2023
@vyasworks
Copy link
Contributor

Looks good, before approving,

Now that we have test cases base setup, can we include a test case for this scenario?

New PR coming in for just the test case to reduce diff size!

@chelsea-EYDS
Copy link
Contributor Author

Merging - previously approved - checked after merging in changes. New PR with tests to follow.

@chelsea-EYDS chelsea-EYDS merged commit 6a5cb20 into main Apr 12, 2023
@chelsea-EYDS chelsea-EYDS deleted the cash branch April 12, 2023 18:35
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.

2 participants