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

Implement parsing of the Amount from the Transaction ID #537

Conversation

yyosifov
Copy link
Contributor

  • For EUR transactions - try parsing the amount from the Transaction Id
  • Added unit tests for the regex and the prepare transaction functions

Closes #1462 (from the frontend repo)

@github-actions
Copy link

github-actions bot commented Aug 15, 2023

✅ Tests will run for this PR. Once they succeed it can be merged.

@yyosifov yyosifov force-pushed the yosifov/extract-transaction-amount-from-id branch from 296a5d2 to 7dde257 Compare August 15, 2023 21:06
@igoychev igoychev added the run tests Allows running the tests workflows for forked repos label Aug 16, 2023
@github-actions github-actions bot removed the run tests Allows running the tests workflows for forked repos label Aug 16, 2023
Copy link
Contributor

@igoychev igoychev left a comment

Choose a reason for hiding this comment

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

Amazing job with the tests and the corner cases!!!
Please see my two comments regarding !=BGN and additional safety with marking the transaction as unrecognized if parsing didn't work.

@sashko9807
Copy link
Member

Assuming that the position of the amount is the exact same on every transactionId, and the amount is always represented as a decimal number, can't we just use transactionId.slice(-16, -9). Should be faster than doing regex matching on the string.

Copy link
Contributor

@igoychev igoychev left a comment

Choose a reason for hiding this comment

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

Great and special thanks again for the extra safety!

debtorAccount: {
iban: 'BG21UNCR111111111111',
},
debtorName: 'Name not relevant for the example',
Copy link
Contributor

Choose a reason for hiding this comment

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

Wow! Great example for setting the high bar of how test data should look like!

@igoychev igoychev added the run tests Allows running the tests workflows for forked repos label Aug 25, 2023
@github-actions github-actions bot removed the run tests Allows running the tests workflows for forked repos label Aug 25, 2023
@yyosifov
Copy link
Contributor Author

@sashko9807 Sorry, I just saw your comment. Here are my thoughts:

  • Regarding performance - I doubt it will be a big difference for so short string and the operation is quite rare - i.e. only happens on a new transaction. I would agree that we should improve readability/maintainability if there is a way :)
  • The slice you suggested looks cool, but it will work only if we have 7 symbols in the Amount. For example, if someone donated 100 EUR it won't parse it correctly. Example: 'Booked_6516347588_70001524349032963FTRO23184809601C20230703100.00_20230703'.slice(-16, -9)' = '3100.00' which is incorrect

@igoychev igoychev merged commit dda9c2e into podkrepi-bg:master Aug 30, 2023
11 of 12 checks passed
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