Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sync data with Google Drive #930

Open
wants to merge 31 commits into
base: sync
Choose a base branch
from

Conversation

SarahRemus
Copy link
Contributor

Related issue

Towards #913

Context / Background

Sync the database from more than one device with TTL installed.
For example, travel with a notebook and load up the data from your home desktop, punch your time, then arrive back home and have the new data synced back to your desktop.

This PR provides the functionality to sync data through Google Drive. It provides only the basic functionalities of importing and exporting to and from Google Drive.

The feature can only be used if authentication with Google is set up
To use the functionality of syncing to Google Drive, we need to create a Google Cloud project for TTL and turn on the Google Drive API. After that, the authentication needs to be set up. The credential.json file should be placed in the highest level of the TTL folder.
For development purposes I did this with my own Google Account, but for production we should use a TTL Google Account.

What change is being introduced by this PR?

This PR enables the user to upload his TTL database to his Google Drive and also import his database from Google Drive into TTL.

  1. I first introduced two new buttons in the Edit menu.

ttl1

  1. When the user first uses this functionality, he will be asked to authenticate himself. Once the app is registered with Google, the user will not see the middle screen. After signing in once, the token file of the user is saved, and he does not have to sign in again.
    ttlauth

  2. After the authentication is done, the user will see this window in his browser. This is only a temporary solution and should be changed in a future PR.
    ttl6

  3. If the user is exporting data, he can now see the TTL JSON file in his Drive, named time_to_leave_export .

  4. If the user is importing data, he can now see the imported data in TTL. The calendar is automatically reloaded. At the moment, the name of the file that is imported is hard-coded as time_to_leave_export . Only a file with this name can be imported. This should also be changed in a future PR.

Explaining implementation details
In the already existing import-export.js file, I added the two functions importDatabaseFromBuffer(..) and getDatabaseAsJSON().
Then I created two new files, import-export-online.js and google-drive.js. The idea behind that is, that in import-export-online.js all functions to import/ export to/from different web services can be defined. I think this will be beneficial once we introduce more web services. google-drive.js stores all Google Drive specific functions. This separation also makes it easier to mock the functions using API calls in testing.

How will this be tested?

For the two new functions in import-export.js, importDatabaseFromBuffer(..) and getDatabaseAsJSON(), I added unit tests in the import-export.js test file.
To test importDatabaseFromGoogleDrive() I mocked the API calls to Google Drive to not slow down the test process too much. And this way I could test my implementation independently of the API.
I'm not sure how to test exportDatabaseToGoogleDrive(..) though.

Manually, the feature can be tested using the workflow described above (after setting up authentication).

Future PRs

As this PR only covers the very basic functionality of syncing data to Google Drive, these are functionalities that should/ could be implemented in the future:

  • Add translation.
  • Validate JSON that should be imported.
  • Add an encryption key that uniquely identifies the app and its files.
  • Encrypt/compress the JSON file.
  • Fix the window that is shown after authentication.
  • Add sing in with Google button.
  • Add option for the user to select a file for the import, maybe with Google Picker API?

I'm looking forward to your feedback @araujoarthur0 @tupaschoal @thamara!

@codecov
Copy link

codecov bot commented Jan 8, 2023

Codecov Report

❗ No coverage uploaded for pull request base (sync@f47ad2f). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             sync     #930   +/-   ##
=======================================
  Coverage        ?   57.18%           
=======================================
  Files           ?       28           
  Lines           ?     2254           
  Branches        ?      331           
=======================================
  Hits            ?     1289           
  Misses          ?      965           
  Partials        ?        0           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@araujoarthur0
Copy link
Collaborator

Thanks for this! I'm trying to find some time soon to review this.

Copy link
Collaborator

@araujoarthur0 araujoarthur0 left a comment

Choose a reason for hiding this comment

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

I tried to test the PR but got this error when trying to import without having exported:

image

Maybe something is missing from package.json?

changelog.md Outdated
@@ -33,6 +33,7 @@
- Fix [#884]: Slightly moving the date picker icon margin in Day view
- Fix [#890]: Leave by field to be updated with removal of entries
- Fix [#892]: start menu entry title on Windows is hyphenated
- Fix [#925]: Punch from tray and reload after changing database do not work
Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel this is unrelated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is related, otherwise the automatic reload of the calendar after importing data does not work

Copy link
Collaborator

Choose a reason for hiding this comment

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

I mean the change in the changelog.md present in this commit. If you rebase your PR on top of main this will disappear.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've never done a rebase before, and I'm slightly scared of it. Could you provide any resources for me on how to do it?

package.json Outdated
@@ -34,7 +34,7 @@
"test": "npm-run-all test:*",
"test:jest": "jest --runInBand --detectOpenHandles --verbose --colors",
"test:mocha": "nyc --reporter=lcov --reporter=json mocha tests/*",
"setup:win" : "npx electron-installer-windows --src ./release-builds/\"Time to Leave-win32-ia32\"/ --dest 'packages/' --verbose --exe \"Time to Leave.exe\""
"setup:win": "npx electron-installer-windows --src ./release-builds/\"Time to Leave-win32-ia32\"/ --dest 'packages/' --verbose --exe \"Time to Leave.exe\""
Copy link
Collaborator

Choose a reason for hiding this comment

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

unrelated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted

js/windows.js Outdated
@@ -49,7 +49,7 @@ function openWaiverManagerWindow(mainWindow, event)
}

/**
* Return the x and y coordinate for a dialog window,
* Return the x and y coordinate for a dialog window,
Copy link
Collaborator

Choose a reason for hiding this comment

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

both unrelated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All these small changes you marked as unrelated are changes introduces by the linter. I strictly followed the development guide and always ran npm run lint-fix to guarantee the code I changed/added follows your rules. Sometimes the linter changed parts of the code I did not work on, so these are the changes you marked as unrelated.
But how can this even happen? Shouldn't all code follow the linting rules? And if I have to check every time I used the linter if it only fixed the parts I'm currently working on and then reverting all the other changes, it becomes almost useless to me because it introduces such an overhead of work? Am I doing something wrong here?

Should I revert all these unrelated linter changes?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry for that 😅 Ideally the repo should already be in a state that guarantees the next person will not have to find new lint issues, and then all linter complaints are only related to your changes. Probably might be a good idea to add the linter to the PR rules. But in the meantime the best is to run the linter separately on main just to fix stuff.
I did try to run npm lint and some of the things shown here were not automatically fixed. Did you also use a different tool like formatting from vscode?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are absolutely right, now that I think about it, it probably was the vscode formatter... Sorry for that! I will revert the changes and have a closer look before pushing changes the next time! :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

No problem! I also fixed in the main branch the issues that npm run lint-fix complains in .js files and the space in package.json.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted

js/menus.js Outdated
@@ -26,9 +27,9 @@ function getMainMenuTemplate(mainWindow)
openWaiverManagerWindow(mainWindow, event);
},
},
{type: 'separator'},
{ type: 'separator' },
Copy link
Collaborator

Choose a reason for hiding this comment

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

unrelated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted

js/calendar.js Outdated Show resolved Hide resolved
}
else if (entry.type === 'regular')
{
// TODO: 'regular' is still here while we allow importing old DB data. Please remove on the next release.
Copy link
Collaborator

Choose a reason for hiding this comment

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

probably not needed anymore.
maybe we should even have removed this already :'(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are there no more 'regular' entries? Only waived and flexible?
Then I could remove the hole else if () block.
And probably also the if (entry.type === 'flexible') if-clause?

If these assumptions are correct, I will also update my test cases in tests__/__main__/invalid-entries-import-drive.js to not contain regular entries anymore (after you confirmed) :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

It was present in the last version of the db, before it became flexible. But since this feature only works with the current version, it won't contain the regular entries.
It has also probably been 2 years since we migrated, and should have removed it 🤣

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

js/import-export.js Outdated Show resolved Hide resolved
__tests__/__main__/regular-import-drive.js Outdated Show resolved Hide resolved
Comment on lines 144 to 145
'2020-2-1': { 'values': ['08:00', '12:00', '13:00', '17:00'] },
'2020-5-3': { 'values': ['08:00', '10:00'] }
Copy link
Collaborator

Choose a reason for hiding this comment

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

many of those and above are unrelated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted

js/menus.js Outdated Show resolved Hide resolved
@araujoarthur0
Copy link
Collaborator

General tip to rebase to main, as a lot of fixes were pushed in december, and to use git rebase -i to merge a few of the commits so the commit structure is more of a timeline of changes.

@araujoarthur0
Copy link
Collaborator

I'm also seeing a importDatabaseFromGoogleDrive text in my terminal from which I start time to leave.

js/import-export.js Outdated Show resolved Hide resolved
@SarahRemus
Copy link
Contributor Author

So after working on the small change requests, it's time to tackle the bigger problems.
Were you at all able to import/ export anything to/from Google Drive?

Your problem looks like the credential file can't be found? Where did you place that file?

I tried to test the PR but got this error when trying to import without having exported:

image

Maybe something is missing from package.json?

@araujoarthur0
Copy link
Collaborator

So after working on the small change requests, it's time to tackle the bigger problems. Were you at all able to import/ export anything to/from Google Drive?

Your problem looks like the credential file can't be found? Where did you place that file?

I tried to test the PR but got this error when trying to import without having exported:
image
Maybe something is missing from package.json?

Oh, I didn't create the credential to test it, totally misread the description. Let me discuss with the team how we'll create a Google account to generate the credentials.

@araujoarthur0
Copy link
Collaborator

@tupaschoal and @thamara FYI. Do we have a TTL email or have we been using thamara's?

@thamara
Copy link
Owner

thamara commented Sep 21, 2023

Hi @SarahRemus, first of all, thanks a lot for this great feature, and sorry it took me so long to get to it.
I have followed the steps described and it works as expected!
But I'm a bit concerned with a few things:

  1. Adding the credentials.json on the repo root file: It seems like something that should not be made public just like some APIs keys. Adding it as a secret in the repo doesn't seem like a possibility as we need the information from the credentials.json at runtime, so it would need to be hardcoded/shipped anyway. There should be a better way of doing this, otherwise other apps would have the same problem. We need to investigate this a bit more.
    This SO question mighe help.

  2. Limitation of filename and location: As my tests showed, whenever we save the database to the drive, it creates a new file with the same name. This can get complicated to maintain as well as making the user's drive a bit dirty. When loading the database, it seems to choose the newer one, but I'm not sure if that's by lucky (we are relying on the position returned by service.files.list).
    Although we can resolve this one later, I would prefer to get it close to its final form before including this change on the release. Releasing as is, can cause problems in the future such as having to maintain two forms of exporting/importing.
    It looks like a good way is to use the concept of app-specific data. We need to better understand this.

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