Skip to content

Conversation

@jvsena42
Copy link
Member

@jvsena42 jvsena42 commented Jun 19, 2025

  • Move remaining method to CurrencyRepo
  • Replace CurrencyService direct access with CurrencyRepo
  • Remove cachedRates: List<FxRate> variable from CurrencyService to follow a single source of true
  • Tests

Continuation of #199

@jvsena42 jvsena42 self-assigned this Jun 19, 2025
@jvsena42 jvsena42 mentioned this pull request Jun 19, 2025
3 tasks
@jvsena42 jvsena42 enabled auto-merge June 19, 2025 16:38
@jvsena42 jvsena42 requested a review from ovitrif June 19, 2025 16:50
Copy link
Collaborator

@ovitrif ovitrif left a comment

Choose a reason for hiding this comment

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

Great work 🎉 , thanks for the effort invested in this refactoring.

Added 1 comment concerning the stale data check

@jvsena42 jvsena42 marked this pull request as draft June 20, 2025 09:48
auto-merge was automatically disabled June 20, 2025 09:48

Pull request was converted to draft

@jvsena42 jvsena42 marked this pull request as ready for review June 20, 2025 10:26
@jvsena42 jvsena42 requested a review from ovitrif June 20, 2025 10:26
@jvsena42 jvsena42 enabled auto-merge June 20, 2025 10:39
@jvsena42 jvsena42 disabled auto-merge June 20, 2025 10:41
@jvsena42 jvsena42 marked this pull request as draft June 20, 2025 10:42
@jvsena42 jvsena42 marked this pull request as ready for review June 20, 2025 10:52
Copy link
Collaborator

@ovitrif ovitrif left a comment

Choose a reason for hiding this comment

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

So much more confidence approving this PR even though it changes parts that are used everywhere throughout the app, all because of the critical unit tests for the conversion logic.

Thanks a lot for those and this refactor 🙇🏻
merging 🚀

@ovitrif ovitrif merged commit 5a1e5c4 into master Jun 20, 2025
1 check passed
@ovitrif ovitrif deleted the refactor/replace-currency-service-access branch June 20, 2025 11:15
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