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

Fix Issue 520 #2517

Merged
merged 1 commit into from
Aug 31, 2023
Merged

Fix Issue 520 #2517

merged 1 commit into from
Aug 31, 2023

Conversation

Tiihi
Copy link
Contributor

@Tiihi Tiihi commented Aug 30, 2023

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.
  • The code builds and is tested on an actual Android device.
  • I confirm that I've run the code locally and everything works as expected.

Pull Request Type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, new lines, etc.)
  • Refactoring (no functional changes, renaming)
  • Small improvement (fix typo, UI fine-tune, change color or something small)
  • Gradle Build related changes
  • Dependencies update (updating libraries)
  • Documentation (clarifying comments, KDoc)
  • Tests (Unit, Integration, UI tests)
  • Other (please describe):

Put an x in all the boxes that apply.

Does this PR closes any GitHub Issues?

Check Ivy Wallet Issues.

What's changed?

Describe with a few bullets what's new:

  • the values are getting now handled depending on the localDecimalSeparator
  • result format in any device language should be "1078.95"
grafik

@Tiihi
Copy link
Contributor Author

Tiihi commented Aug 30, 2023

FYI i will provide tmrw some screen recordings for the export process.

@ILIYANGERMANOV ILIYANGERMANOV self-requested a review August 31, 2023 08:57
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.

Awesome work @Tiihi! The fix LGTM 💯
Yeah, please send the screen recordings and do a smoke test for Export & Import Ivy CSV (make sure to include an account holding BTC or other crypto)

If everything works, I'll merge it and it'll be live in the next release 🙌

@Tiihi
Copy link
Contributor Author

Tiihi commented Aug 31, 2023

Awesome work @Tiihi! The fix LGTM 💯 Yeah, please send the screen recordings and do a smoke test for Export & Import Ivy CSV (make sure to include an account holding BTC or other crypto)

If everything works, I'll merge it and it'll be live in the next release 🙌

Hi @ILIYANGERMANOV,

here are the recordings for the import / export process in two different device languages (german & english). I also added the two export csv-files to this post.

I restarted the app between every process and deleted of course the local user data before importing the csv files. I think i also found another bug that appears when i delete the local user data and try to import the files afterwards without restarting the app. The App is stuck after successfully importing the data and i cant continue to set my currency. This Bug doesnt have anything to do with my changes, i also tested this behavior on the current main branch without my changes. I will create a bug ticket if none exists yet.

German Import / Export / CSV File:

german_export.mp4
german_import.mp4

german.csv

English Import / Export / CSV File:

english_export.mp4
english_import.mp4

english.csv

@Tiihi
Copy link
Contributor Author

Tiihi commented Aug 31, 2023

Created #2519 for the issue that i discovered while testing this bug fix

@ILIYANGERMANOV
Copy link
Collaborator

Excellent work! Thanks for raising the bug ticket 🐛 Merging to main

@ILIYANGERMANOV ILIYANGERMANOV merged commit 99afd8e into Ivy-Apps:main Aug 31, 2023
@Tiihi Tiihi deleted the fix-issue-520 branch August 31, 2023 17:49
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CSV export broken on locales using comma as decimal separator
2 participants