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

fix csv export #3444

Merged
merged 11 commits into from
Sep 1, 2024
Merged

fix csv export #3444

merged 11 commits into from
Sep 1, 2024

Conversation

akashs056
Copy link
Contributor

@akashs056 akashs056 commented Aug 30, 2024

Pull request (PR) checklist

Please check if your pull request fulfills the following requirements:

  • I've read the Contribution Guidelines and my PR doesn't break the rules.
  • I've read and understand the Developer Guidelines.
  • I confirm that I've run the code locally and everything works as expected.
  • My PR includes only the necessary changes to fix the issue (i.e., no unnecessary files or lines of code are changed).
  • 🎬 I've attached a screen recording of using the new code to the next paragraph (if applicable).

Screen recording of your changes (if applicable):

What's changed?

Describe with a few bullets what's new:

fixed this issue #3422 previously by #3426
but after the time issue if fixed by #3435 its works incorrectly it exports time at UTC and does not converts it to Local time
So, with this changes its now working perfectly fine exports in users local time and aslo imports in local time zone without any issue

Before After
{media} {media}
{media} {media}

Risk factors

What may go wrong if we merge your PR?

  • ...
  • ...

In what cases won't your code work?

  • ...
  • ...

Does this PR close any GitHub issues? (do not delete)

Troubleshooting GitHub Actions (CI) failures ❌

Pull request checks 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.

Left some comments:

  1. Export CSV time in UTC
  2. Use the TimeConverter API

@ILIYANGERMANOV ILIYANGERMANOV added the requested changes Changes are needed. Please, apply them label Aug 30, 2024
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! 💯

@ILIYANGERMANOV
Copy link
Collaborator

@akashs056 you neee to fix the ExportCsvUseCase unit tests by providing a mocked TimeConverter impl

@ILIYANGERMANOV
Copy link
Collaborator

@akashs056 you neee to fix the ExportCsvUseCase unit tests by providing a mocked TimeConverter impl

Or even better and easier, create a TestTimeConverter impl that converts always to UTC. I mean in it you hard-code the current timezone to UTC. Does that makes sense?

@akashs056
Copy link
Contributor Author

@ILIYANGERMANOV, I've implemented a TestTimeConverter that hard-codes the current timezone to UTC as you suggested. Could you please confirm if this is the correct approach? I'm still new to unit testing, so I want to make sure I'm on the right track.

@ILIYANGERMANOV
Copy link
Collaborator

@akashs056 you have a build error
e: file:///home/runner/work/ivy-wallet/ivy-wallet/shared/base/src/main/java/com/ivy/base/time/impl/TestTimeConverter.kt:9:1 Class 'TestTimeConverter' is not abstract and does not implement abstract member 'toLocalTime'.

The TestTimeConveter will be red

@akashs056
Copy link
Contributor Author

@akashs056 you have a build error e: file:///home/runner/work/ivy-wallet/ivy-wallet/shared/base/src/main/java/com/ivy/base/time/impl/TestTimeConverter.kt:9:1 Class 'TestTimeConverter' is not abstract and does not implement abstract member 'toLocalTime'.

The TestTimeConveter will be red

I've resolved the initial error with the TestTimeConverter class in my local build. However, now I'm encountering a UncompletedCoroutinesError in the test property - num of row and columns matches the format. It seems to be related to the coroutine not completing within the expected time

@ILIYANGERMANOV
Copy link
Collaborator

@akashs056 push it so I can have a look - the CI will also provide feedback

@akashs056
Copy link
Contributor Author

@ILIYANGERMANOV getting the same error again

@akashs056
Copy link
Contributor Author

akashs056 commented Aug 31, 2024

sometimes when i run locally with the same code it does not gives any error
image

@ILIYANGERMANOV
Copy link
Collaborator

@akashs056 fix build errors, update branch and tests should pass on the CI 🤞 I don't see anything incorrect

@akashs056
Copy link
Contributor Author

@ILIYANGERMANOV Gradle error indicating that the toLocalTime method is not implemented, but I can’t find this method in the TimeConverter interface. The error has been persisting since yesterday, and I’m having trouble resolving it. Could you please help me understand what might be going wrong?

@ILIYANGERMANOV
Copy link
Collaborator

@ILIYANGERMANOV Gradle error indicating that the toLocalTime method is not implemented, but I can’t find this method in the TimeConverter interface. The error has been persisting since yesterday, and I’m having trouble resolving it. Could you please help me understand what might be going wrong?

It's there, look into StandardTimeConveter. It was merged yesterday - update branch, git pull and open the implementation

@ILIYANGERMANOV
Copy link
Collaborator

@akashs056 now git pull and implement TestTimeConverter and everything should be ✅

@akashs056
Copy link
Contributor Author

@ILIYANGERMANOV Its ready to merge

@ILIYANGERMANOV ILIYANGERMANOV merged commit 607f6b2 into Ivy-Apps:main Sep 1, 2024
9 checks passed
@akashs056 akashs056 deleted the fix-csvexport branch September 1, 2024 12:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
requested changes Changes are needed. Please, apply them
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Incorrect time shown in exported csv file
3 participants