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

Update Transaction to avoid serde errors #16

Merged
merged 5 commits into from
Jul 9, 2021

Conversation

BreD1810
Copy link
Contributor

@BreD1810 BreD1810 commented Jul 9, 2021

This PR:

  • Removes account_balance field on Transaction - although this is still shown on the Monzo doc examples, it does not appear to be returned by the API any more?
  • Changes Category from an enum to a String (as suggested in transaction categories are not exhaustive #12).
  • Makes the ID variant of MerchantInfo an Option<String> (the API can return null values, for example with a top-up).

@BreD1810 BreD1810 changed the title Update Transaction Update Transaction to avoid serde errors Jul 9, 2021
@codecov
Copy link

codecov bot commented Jul 9, 2021

Codecov Report

Merging #16 (6f4edf7) into master (1f412da) will increase coverage by 3.72%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #16      +/-   ##
=========================================
+ Coverage    3.57%   7.29%   +3.72%     
=========================================
  Files          17      18       +1     
  Lines         224     233       +9     
=========================================
+ Hits            8      17       +9     
  Misses        216     216              
Impacted Files Coverage Δ
src/endpoints/transactions.rs 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1f412da...6f4edf7. Read the comment docs.

@danieleades
Copy link
Owner

is it possible to add some unit tests to show the deserialisation works? I would recommend grabbing the raw JSON from the API playground, anonymising it, and then showing that it's correctly deserialised.

you would have to extract the logic in the send method into a function that you can unit test.

BreD1810 added 2 commits July 9, 2021 11:13
Revert MerchantInfo's ID variant to be String.
Includes:
- Transaction with expanded details.
- Transaction with null merchant.
- Multiple transactions.
@BreD1810
Copy link
Contributor Author

BreD1810 commented Jul 9, 2021

is it possible to add some unit tests to show the deserialisation works?

I've added (anonymised) unit tests for:

  • Expanded transaction
  • Transaction with null merchant
  • Multiple transactions in a single request.

Copy link
Owner

@danieleades danieleades left a comment

Choose a reason for hiding this comment

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

i'm happy with these changes. so long as you promise the test data is properly anonymised. I'd hate to inadvertently share any personal information

@BreD1810
Copy link
Contributor Author

BreD1810 commented Jul 9, 2021

Triple checked none of the identifying values are actual ones - just tried to keep them similar to actual values that may be returned.

I essentially used ascending numbers/letters for each value, making sure the digits were in the correct location as returned by the real response. Eg:

"id": "tx_0000A1aBC2Dbc34Ede5fEH",

@danieleades danieleades merged commit 98a4a59 into danieleades:master Jul 9, 2021
@danieleades
Copy link
Owner

brilliant work @BreD1810. Thanks again for the pull request!

@BreD1810 BreD1810 deleted the transaction-fix branch July 9, 2021 14:12
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