Feature google drive #2787

mohammednawas8 · 2023-10-12T12:58:25Z

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 "Contributing 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.
https://github.com/Ivy-Apps/ivy-wallet/assets/78867217/5a972c68-ffbe-423d-b292-c20ce548f15a

What's changed?

Describe with a few bullets what's new:
a Adding new option in the settings screen which is Google Drive backup
b Ability to link a google drive account to backup the user's data each 12 hours
c Ability to delete/change the drive account.

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

Risk Factors

What may go wrong if we merge your PR?

a
b
c

In what cases your code won't work?

a
b
c

Does this PR closes any GitHub Issues?

Check Ivy Wallet Issues.

Closes [FEATURE] Google Drive auto-backups #2709

Replace ISSUE_NUMBER with your issue number (for example Closes #1234). If you've done that correctly, you'll see the issue title linked when previewing your PR description.

Troubleshooting CI failures

If you see any of the PR checks failing (❌) go to Actions and find it there. Or simply click "Details" next to the failed check and explore the logs to see why it has failed.

Detekt

Detekt is a static code analyzer for Kotlin that we use to enforce code readibility and good practices.

To run Detekt locally:

./gradlew detekt

If the Detekt errors are caused by a legacy code, you can suppress them using a basline.

Detekt baseline (not recommended)

./scripts/detektBaseline.sh

Lint

We use the standard Android Lint plus Slack's compose-lints as an addition to enforce proper Compose usage.

To run Lint locally:

./scripts/lint.sh

If the Lint errors are caused by a legacy code, you can suppress them using a basline.

Lint baseline (not recommended)

./scripts/lintBaseline.sh

Unit tests

If this job is failing this means that your changes break an existing unit test. You must identify the failing tests and fix your code.

To run the Unit tests locally:

./gradlew testDebugUnitTest

ILIYANGERMANOV

Hey @mohammednawas8 thank you for your hard work! 🎉 Your code seems working but it we need to make some changes to make it mergeable.

You need to address:

proper error handling using Arrow (https://github.com/Ivy-Apps/ivy-wallet/blob/deprecated-ivywallet-rewrite-2223/drive/google-drive/src/main/java/com/ivy/drive/google_drive/GoogleDriveServiceImpl.kt)
Don't suppress Detekt errors in your module - fix them.
Add Unit tests using Mockk for all classes
Ideally, it should look like the implementation which we did last year:
https://github.com/Ivy-Apps/ivy-wallet/tree/deprecated-ivywallet-rewrite-2223/drive/google-drive/src/main/java/com/ivy/drive/google_drive

I've pasted in the issue for reference because it features solid error handling + good type-safety.

No rush, man. If you're near the situation in Israel, please stay safe. Hope this ends soon 🙏

ILIYANGERMANOV · 2023-10-12T14:15:09Z

.idea/codeStyles/Project.xml

Remove + add to .gitignore

ILIYANGERMANOV · 2023-10-12T14:15:15Z

.idea/codeStyles/codeStyleConfig.xml

Remove + add to .gitignore

ILIYANGERMANOV · 2023-10-12T14:15:20Z

.idea/deploymentTargetDropDown.xml

Remove + add to .gitignore

ILIYANGERMANOV · 2023-10-12T14:15:26Z

.idea/shelf/Uncommitted_changes_before_Checkout_at_18_09_2023/shelved.patch

Remove + add to .gitignore

ILIYANGERMANOV · 2023-10-12T14:16:14Z

app/src/main/java/com/ivy/wallet/IvyAndroidApp.kt

+
+// This solves Could not instantiat worker issue
+class CustomWorkerFactory @Inject constructor(


Is there a better solution to this? This seems like a hack that'll become tech debt in the future

ILIYANGERMANOV · 2023-10-12T14:19:54Z

google-drive/src/main/java/com/ivy/googledrive/backup/BackupWorker.kt

+                backupFile,
+                "${IVY_BACKUP_FILE_NAME}-${Date()}",
+                DriveMimeType.JSON,
+                5


Magic Number, Detekt raised a valid error

ILIYANGERMANOV · 2023-10-12T14:21:27Z

google-drive/src/main/java/com/ivy/googledrive/backup/BackupWorker.kt

+    // The reason of why i did not use hilt to inject this repo in the constructor is because i want to get the drive instance during
+    // The runtime, if we use hilt to inject this in the constructor the drive instance will be null when the user links his google account
+    // For the first time.
+    private fun getDriveBackRepository(context: Context): DriveBackupRepository {


You can use @Provides + dagger.Lazy like Lazy<DriveBackupRepository - let's use Hilt for that

ILIYANGERMANOV · 2023-10-12T14:21:47Z

google-drive/src/main/java/com/ivy/googledrive/backup/DriveBackupConstants.kt

+    internal const val IVY_FOLDER_NAME = "Ivy"
+    const val IVY_BACKUP_FILE_NAME = "Ivy-backup"
+    const val WORKER_TAG = "Ivy-Backup-Worker"
+


Needless blank lines, Detekt has caught that too

ILIYANGERMANOV · 2023-10-12T14:22:38Z

google-drive/src/main/java/com/ivy/googledrive/backup/DriveBackupRepository.kt

+     * @param n is The capacity of the Backup folder,
+     * if the capacity is full then the oldest file will get deleted and get replaced by the new file.
+     * */
+    @Throws(Exception::class)


That's very bad! Throwing exceptions leads to unexpected crash. Please use Arrow's Either like we did in https://github.com/Ivy-Apps/ivy-wallet/blob/deprecated-ivywallet-rewrite-2223/drive/google-drive/src/main/java/com/ivy/drive/google_drive/GoogleDriveServiceImpl.kt

I catch the exceptions using try and catch

Hey @mohammednawas8 that works but the problem is that Exceptions are prone to errors and runtime crashes. We work with typed errors:
https://arrow-kt.io/learn/typed-errors/working-with-typed-errors/

TL;DR;

If a function is expected to fail, e.g. Network request (it happens often) we model its return type explicitly with Either

We throw Exceptions only for exceptional cases - situations which we don't expect to happen. For example, save to DB failing because the device is out of storage or some IO error.

Does that make sense?

This can provide more context:
https://www.linkedin.com/posts/iliyan-germanov-3963b5b9_working-with-typed-errors-arrow-activity-7115666238504103937-d5df?utm_source=share&utm_medium=member_android

i will take a look at that

ILIYANGERMANOV · 2023-10-12T14:23:02Z

google-drive/src/main/java/com/ivy/googledrive/backup/DriveBackupRepository.kt

+        file: File,
+        fileName: String,
+        mimeType: DriveMimeType,
+        n: Int


What is n? 😄

it is the backup folder capacity, you mentioned it in the requirements

It should make up to N backup files

When N is reached the oldest backup is deleted.
but anyways i changed its name to be clearer, there is a documentation in the DriveBackRepository interface that explains thethisattribute.

ILIYANGERMANOV · 2023-10-12T14:28:47Z

Hey @mohammednawas8 we also need to indicate whether Google Drive backups are enabled in Settings or not. Maybe a Switch? Or... some UI telling the user whether backups are enabled.

mohammednawas8 · 2023-10-12T17:45:55Z

Did you watch the attached video
https://github-production-user-asset-6210df.s3.amazonaws.com/78867217/274605831-5a972c68-ffbe-423d-b292-c20ce548f15a.mp4

as you can notice if the user has already linked his drive account with the app and tried to click on the google drive backup option again, a dialog will show up that shows the back up email and the ability to delete the account.

ILIYANGERMANOV · 2023-10-12T17:47:46Z

Did you watch the attached video https://github-production-user-asset-6210df.s3.amazonaws.com/78867217/274605831-5a972c68-ffbe-423d-b292-c20ce548f15a.mp4

as you can notice if the user has already linked his drive account with the app and tried to click on the google drive backup option again, a dialog will show up that shows the back up email and the ability to delete the account.

the user must be able to notice that w/o having to click. Like having different UI states on the button. If enabled for example a green pill saying "Enabled"

mohammednawas8 · 2023-10-12T18:28:53Z

okay i will work on it

mohammednawas8 · 2023-10-12T19:21:05Z

I added a green text in the same option
https://github.com/Ivy-Apps/ivy-wallet/assets/78867217/695408f3-daba-4e53-a46e-8a2b0d97191d

ILIYANGERMANOV · 2023-10-12T19:25:32Z

I added a green text in the same option https://github.com/Ivy-Apps/ivy-wallet/assets/78867217/695408f3-daba-4e53-a46e-8a2b0d97191d

Remove the ✔️ and use only "Enabled" with 1dp border radius green. Other than that looks good!

mohammednawas8 · 2023-10-12T19:56:49Z

Like this

ILIYANGERMANOV · 2023-10-12T19:58:21Z

Like this

It's a perfect! Just do

vertical padding 4.dp or 8.dp
horizontal 16.dp

Mohammad Nawas added 4 commits October 12, 2023 15:28

gradle

ec902c0

Google Drive Backup

99b28b6

baseline file changes

bf445a4

Add Detekt baseline

0421ae2

mohammednawas8 requested a review from ILIYANGERMANOV as a code owner October 12, 2023 12:58

Conflicts

9ada538

ILIYANGERMANOV suggested changes Oct 12, 2023

View reviewed changes

Adding enabled text for google drive backup option

6eaa433

github-actions bot added the Stale No activity, will be automatically closed soon. label Oct 15, 2023

github-actions bot closed this Oct 16, 2023

ILIYANGERMANOV reopened this Oct 18, 2023

github-actions bot added Stale No activity, will be automatically closed soon. and removed Stale No activity, will be automatically closed soon. labels Oct 19, 2023

github-actions bot closed this Oct 23, 2023

ILIYANGERMANOV mentioned this pull request Jan 15, 2024

Automatic backup to Google drive depending on frequency set by user[FEATURE] #2866

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature google drive #2787

Feature google drive #2787

mohammednawas8 commented Oct 12, 2023

ILIYANGERMANOV left a comment

ILIYANGERMANOV Oct 12, 2023

ILIYANGERMANOV Oct 12, 2023

ILIYANGERMANOV Oct 12, 2023

ILIYANGERMANOV Oct 12, 2023

ILIYANGERMANOV Oct 12, 2023

ILIYANGERMANOV Oct 12, 2023

ILIYANGERMANOV Oct 12, 2023

ILIYANGERMANOV Oct 12, 2023

ILIYANGERMANOV Oct 12, 2023

mohammednawas8 Oct 18, 2023

ILIYANGERMANOV Oct 18, 2023

ILIYANGERMANOV Oct 18, 2023

mohammednawas8 Oct 18, 2023

ILIYANGERMANOV Oct 12, 2023

mohammednawas8 Oct 12, 2023 •

edited

Loading

ILIYANGERMANOV commented Oct 12, 2023

mohammednawas8 commented Oct 12, 2023

ILIYANGERMANOV commented Oct 12, 2023

mohammednawas8 commented Oct 12, 2023

mohammednawas8 commented Oct 12, 2023

ILIYANGERMANOV commented Oct 12, 2023 •

edited

Loading

mohammednawas8 commented Oct 12, 2023

ILIYANGERMANOV commented Oct 12, 2023


		// This solves Could not instantiat worker issue
		class CustomWorkerFactory @Inject constructor(

Feature google drive #2787

Feature google drive #2787

Conversation

mohammednawas8 commented Oct 12, 2023

Pull Request (PR) Checklist

What's changed?

Risk Factors

Does this PR closes any GitHub Issues?

Troubleshooting CI failures

Detekt

Lint

Unit tests

ILIYANGERMANOV left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mohammednawas8 Oct 12, 2023 • edited Loading

Choose a reason for hiding this comment

ILIYANGERMANOV commented Oct 12, 2023

mohammednawas8 commented Oct 12, 2023

ILIYANGERMANOV commented Oct 12, 2023

mohammednawas8 commented Oct 12, 2023

mohammednawas8 commented Oct 12, 2023

ILIYANGERMANOV commented Oct 12, 2023 • edited Loading

mohammednawas8 commented Oct 12, 2023

ILIYANGERMANOV commented Oct 12, 2023

mohammednawas8 Oct 12, 2023 •

edited

Loading

ILIYANGERMANOV commented Oct 12, 2023 •

edited

Loading