Skip to content
This repository has been archived by the owner on Nov 5, 2024. It is now read-only.

Fix issue 2762 #2926

Merged
merged 35 commits into from
Feb 8, 2024
Merged

Fix issue 2762 #2926

merged 35 commits into from
Feb 8, 2024

Conversation

Rick-AB
Copy link
Contributor

@Rick-AB Rick-AB commented Feb 7, 2024

Pull Request (PR) Checklist

Please check if your pull request fulfills the following requirements:

  • The PR is submitted to the main branch.
  • I've read the Contribution Guidelines and my PR doesn't break the rules.
  • I've read the Architecture Guidelines.
  • I confirm that I've run the code locally and everything works as expected.
  • 🎬 I've attached a screen recoding of the changes.

Tip: drag & drop the video to the PR description.

What's changed?

Describe with a few bullets what's new:

  • Includes new transaction model mapper
  • Includes transaction repository implementation
  • Includes transaction repository tests

💡 Tip: Please, attach screenshots and screen recordings. It helps a lot!

Risk Factors

What may go wrong if we merge your PR?

  • N/A (transaction repo is not currently being used. planned as a future implementation)

In what cases your code won't work?

  • N/A

Does this PR closes any GitHub Issues?

Check Ivy Wallet Issues.

Replace ISSUE_NUMBER with the number of the GitHub issue that this PR is fixing. If you've done that correctly, you'll see the issue title linked when previewing your PR description.

!Note: Do not link the PR number. Link the number/id of the GitHub Issue from issues.

Troubleshooting CI failures ❌

GitHub Actions failing? Read our CI Troubleshooting guide.

Copy link
Collaborator

@ILIYANGERMANOV ILIYANGERMANOV left a comment

Choose a reason for hiding this comment

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

Overall LGTM! A lof time has passed since we initially opened the data layer tasks and some things have changed. A few things to address:

  1. Fix all CI errors (most of them seem to be code formatting & styling)
  2. Remove the datasource + the fakes in :data-testing
  3. Fox inconsistencies in the mapper (https://github.com/Ivy-Apps/ivy-wallet/blob/data-layer-part-2/shared/data/src/main/java/com/ivy/data/repository/mapper/TransactionMapper.kt for reference)

Haven't reviewed fully but things seems good!

@ILIYANGERMANOV ILIYANGERMANOV added the keep Keep it from automatically getting closed by Stale label Feb 7, 2024
@ILIYANGERMANOV
Copy link
Collaborator

@Rick-AB feel free to suppress the Detekt erros

Copy link
Collaborator

@ILIYANGERMANOV ILIYANGERMANOV left a comment

Choose a reason for hiding this comment

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

Nice work! 💯 Thank you for making the changes @Rick-AB :) Merging after the CI is green ✅

@ILIYANGERMANOV ILIYANGERMANOV merged commit 22eacd5 into Ivy-Apps:main Feb 8, 2024
8 checks passed
@Rick-AB Rick-AB deleted the fix-issue-2762 branch February 8, 2024 21:27
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
keep Keep it from automatically getting closed by Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Data Layer: Transaction Repository
2 participants