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

✉️📭 Refractored emailService.ts into shareLocalDBFile.ts #1160

Merged
merged 15 commits into from
Jul 8, 2024

Conversation

the-bay-kay
Copy link
Contributor

@the-bay-kay the-bay-kay commented May 21, 2024

Purpose

Because of the several document problems with the previous email service, most recent being Issue 1047, I am updating emailServices.ts to use the more flexible socialSharing plugin that is used in controlHelper.ts. To reflect these updates, the service was renamed, and the English i18next entries updated.

Recent Changes:

  • Moved into services/ directory
  • Refractored emailService.ts to now use sharePlugin, to match controlHelper
  • To reflect this change, renamed the service to shareLocalDBFile
  • Reworked 18next translations to reflect change
  • Removed old email plugin from package-cordovabuild.json

Remaining Tests

  • Build for iOS
  • Build for Android
  • Test on iOS Emulation
  • Test on iOS Hardware
  • Test on Android Hardware

- Moved into `services/` directory
- Refractored `emailService.ts` to now use sharePlugin, to match controlHelper
- To reflect this change, renamed the service to `shareLocalDBFile`
- Reworked 18next translations to reflect change
@the-bay-kay
Copy link
Contributor Author

Testing Updates

When running on both iOS and Android, the share function works, with a notable exception.

Below are two screenshots: one sending logs via the Apple Mail app, and the second via GMail. As can be seen, the loggerDB file cannot be attatched to a GMail message. This error occurs on both iOS and Android. We can, however, send the file through several other means - Google Drive, Apple Mail, etc.

My first theory as to why this occurs: Gmail prohibits the sending of certain files (link). Because the loggerDB file has no extension, Gmail "plays it safe", and does not allow us to attach it.

What is strange, however, is that this error does not occur with the old email plugin. Why would Gmail prohibit the attachment in one circumstance, and not another? From what I can gather from the working sharing (Google Drive, Apple Mail), the file itself is formed and sent correctly... I'll do a bit more digging

- Issues with socialSharing, `loggerDB` file, and Gmail App
- Because Gmail is default mail on Android, we want to make sure it works
- Reverting to old plugin to make sure this sends correctly
- Tested and confirmed send works on iOS & Android Hardware
- Please see PR for further details
Copy link

codecov bot commented May 22, 2024

Codecov Report

Attention: Patch coverage is 0% with 63 lines in your changes missing coverage. Please review.

Project coverage is 30.36%. Comparing base (e55e5ae) to head (f0656ca).
Report is 3 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1160      +/-   ##
==========================================
- Coverage   30.59%   30.36%   -0.24%     
==========================================
  Files         118      118              
  Lines        5216     5256      +40     
  Branches     1154     1106      -48     
==========================================
  Hits         1596     1596              
- Misses       3618     3658      +40     
  Partials        2        2              
Flag Coverage Δ
unit 30.36% <0.00%> (-0.24%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
www/js/services/controlHelper.ts 0.00% <ø> (ø)
www/js/control/LogPage.tsx 0.00% <0.00%> (ø)
www/js/control/ProfileSettings.tsx 0.00% <0.00%> (ø)
www/js/control/SensedPage.tsx 0.00% <0.00%> (ø)
www/js/services/shareLocalDBFile.ts 0.00% <0.00%> (ø)

... and 1 file with indirect coverage changes

@the-bay-kay
Copy link
Contributor Author

Quick Updates:

  • It seems that this latest changed failed the prettier action, but running npx prettier --write www/ does not change anything for me... Checking my prettier version to make sure I'm on the right version
  • Because of the email issues described above, I reverted the android portion of this service to the older plugin. Gmail now works as intended on Android (i.e., works as it did before)

@shankari
Copy link
Contributor

@the-bay-kay is a workaround to simply rename the file as .pdf before trying to attach it

This reverts commit 7704af9.

Reverting to restore parity on iOS / Android
This reverts commit 3e0ddb8.

Reverting to restory parity on iOS / Android
@the-bay-kay
Copy link
Contributor Author

the-bay-kay commented Jun 26, 2024

... is a workaround to simply rename the file as .pdf before trying to attach it

Theoretically, I believe so. Attempting to do so now, but re-naming a file has become a non-trivial task: We need to use the Cordova File Plugin to (i) read the contents, (ii) write a new .txt file, (iii) send the temp file via the plugin, and (iv) clean up the temp file, as we do in controlHelper.

Currently wrestling with the Cordova file plugin right now, stumped as to what's causing the following issue:

When running the existing code:

if (window['cordova'].platformId == 'android') {
    filePath = 'app://databases/' + database;
  } else if (window['cordova'].platformId == 'ios') {
    logDebug(window['cordova'].file.dataDirectory);
    filePath = window['cordova'].file.dataDirectory + '../LocalDatabase/' + database;
  } else { // == 'unknown'
    displayErrorMsg('Error: Unknown OS!');
    return new Error('Unknown OS!');
  }


  logDebug(`FilePath is: ${filePath}`)

  const shareObj = {
    files: [filePath],
    message: i18next.t('shareFile-service.send-log.body-please-fill-in-what-is-wrong'),
    subject: i18next.t('shareFile-service.send-log.subject-logs'),
  };

The file path is printed as follows:

DEBUG: FIlePath is: file:///Users/{user}/Library/Developer/CoreSimulator/Devices/{ID}/data/Containers/Data/Application/{ID}/Library/NoCloud/../LocalDatabase/loggerDB

When attempting to access this URL the same way, using the Cordova File Plugin:

async function localReadFile() {
    return new Promise<void>((resolve, reject) => {
      let pathToFile;
      if (window['cordova'].platformId == 'android') {
        pathToFile = 'app://databases/' + fileName;
      } else if (window['cordova'].platformId == 'ios') {
        pathToFile  = window['cordova'].file.dataDirectory + '../LocalDatabase/' + fileName;
        logDebug(`PathToFile: ${pathToFile}`);
      } else { // == 'unknown'
        displayErrorMsg('Error: Unknown OS!');
        reject();
      }
      window['resolveLocalFileSystemURL'](pathToFile, (fs) => {
        fs.filesystem.root.getFile(fileName, { create: false, exclusive: false }, (fileEntry) => {
          logDebug(`fileEntry ${fileEntry.nativeURL} is file? ${fileEntry.isFile.toString()}`);

          fileEntry.file(
            (file) => {
              const reader = new FileReader();
              logDebug(`fileEntry ${JSON.stringify(file, null, 2)}`);

              reader.onloadend = () => {
                const readResult = this.result;
                logDebug(`File Contents: ${JSON.stringify(readResult, null, 2)} `)
              };
              reader.readAsText(file);
              // The code continues...

We get the following output:

DEBUG:fileEntry file:///Users/{user}/Library/Developer/CoreSimulator/Devices/{ID}/data/Containers/Data/Application/{ID}/Library/loggerDB is file? true

It seems the relative path we're constructing is not getting resolved:

  • .../Application/{ID}/Library/NoCloud/../LocalDatabase/loggerDB
  • .../Application/{ID}/Library/loggerDB

Why would this happen? Trying to go through the plugin docs for an answer...

@the-bay-kay
Copy link
Contributor Author

I think there are some obvious holes in my logic here -- when this code executes:

      window['resolveLocalFileSystemURL'](pathToFile, (fs) => {
        fs.filesystem.root.getFile(fileName, { create: false, exclusive: false }, (fileEntry) => {
          logDebug(`fileEntry ${fileEntry.nativeURL} is file? ${fileEntry.isFile.toString()}`);

The "fs.filesystem.root" is set to the path of the file itself -- It'd be more sensical to instead point somewhere a bit higher, and then use the file name to navigate down. This still doesn't solve answer (i) why we can't set the root using relative paths, and (ii) how we can use the cordova builtin directories to navigate to the file. We need to set the "root" one above cordova['window'].file.dataDirectory -- how can we do so if we cannot navigate via a relative path? Need to do more reading.

@the-bay-kay
Copy link
Contributor Author

Well, you miss obvious things working late... Like my previous hunch, the issue was with setting the root directly to the file. If we set it like this...

      let pathToFile, parentDirectory;
      if (window['cordova'].platformId == 'android') {
        parentDirectory = window['cordova'].file.dataDirectory;
        pathToFile = fileName;
      } else if (window['cordova'].platformId == 'ios') {
        parentDirectory = window['cordova'].file.dataDirectory + '../';
        pathToFile = 'LocalDatabase/' + fileName;
      } else { // == 'unknown'
        displayErrorMsg('Error: Unknown OS!');
        return new Error('Unknown OS!');
      }
      window['resolveLocalFileSystemURL'](parentDirectory, (fs) => {
        fs.filesystem.root.getFile(pathToFile, { create: false, exclusive: false }, (fileEntry) => {
          logDebug(`fileEntry ${fileEntry.nativeURL} is file? ${fileEntry.isFile.toString()}`);
          // ... More Code Below

We successfully find the file:

 [console.log] DEBUG:fileEntry file:///Users/{user}/Library/Developer/CoreSimulator/Devices/{ID}/data/Containers/Data/Application/{ID}/Library/LocalDatabase/loggerDB is file? true
 [console.log] DEBUG:fileEntry {
   "name": "loggerDB",
   "localURL": "cdvfile://localhost/library/LocalDatabase/loggerDB",
   "type": "application/octet-stream",
   "lastModified": 1719418674276.2551,
   "lastModifiedDate": 1719418674276.2551,
   "size": 1104138240,
   "start": 0,
   "end": 1104138240
 }

@the-bay-kay
Copy link
Contributor Author

...we successfully find the file:

The FileEntry is correct now, but we're still not successfully reading the file: progress is progress, now I have the next piece to investigate...

File Reader Code + Console Output
          // More code above & below...
          logDebug(`fileEntry ${fileEntry.nativeURL} is file? ${fileEntry.isFile.toString()}`);
          fileEntry.file(
            (file) => {
              const reader = new FileReader();
              logDebug(`fileEntry ${JSON.stringify(file, null, 2)}`);
              reader.onloadend = () => {
                const readResult = this.result;
                logDebug(`File Contents: ${JSON.stringify(readResult, null, 2)} `)
[0] [phonegap] [console.log] DEBUG:File Contents: undefined 

@the-bay-kay
Copy link
Contributor Author

Attempting a different approach, utilizing the Cordova File Plugin's copyTo. Code is included below the cut: When running the code, I get the subsequent error...

TS For File Copy
  async function localCopyFile() {
    return new Promise<void>((resolve, reject) => {
      let pathToFile, parentDirectory;
      if (window['cordova'].platformId == 'android') {
        parentDirectory = window['cordova'].file.dataDirectory;
        pathToFile = fileName;
      } else if (window['cordova'].platformId == 'ios') {
        parentDirectory = window['cordova'].file.dataDirectory + '../';
        pathToFile = 'LocalDatabase/' + fileName;
      } else { // == 'unknown'
        displayErrorMsg('Error: Unknown OS!');
        throw new Error('Unknown OS!');
      }

      window['resolveLocalFileSystemURL'](parentDirectory, (fs) => {
        fs.filesystem.root.getFile(pathToFile, { create: false, exclusive: false }, (fileEntry) => {
          logDebug(`fileEntry ${fileEntry.nativeURL} is file? ${fileEntry.isFile.toString()}`);
          
          window['resolveLocalFileSystemURL'](window['cordova'].file.cacheDirectory, (copyDir) => {
            logDebug(`DirObject?: ${JSON.stringify(copyDir.filesystem.root, null, 2)}`)

            fileEntry.copyTo(copyDir.filesystem.root, fileName + '.txt', (res) => {
              logDebug(`Res: ${res}`)
            }, (rej)=> {
              logDebug(`Rej: ${JSON.stringify(rej, null, 2)}`)
            })
          });
          // Code follows...
[console.log] DEBUG:fileEntry file:///Users/{USER}/Library/Developer/CoreSimulator/Devices/{ID}/data/Containers/Data/Application/{ID}/Library/LocalDatabase/loggerDB is file? true
[console.log] DEBUG:DirObject?: {
  "isFile": false,
  "isDirectory": true,
  "name": "/",
  "fullPath": "/",
  "filesystem": "<FileSystem: cache>",
  "nativeURL": "file:///Users/{User}/Library/Developer/CoreSimulator/Devices/{ID}/data/Containers/Data/Application/{ID}/Library/Caches/"
}
[console.log] DEBUG:Rej: {
  "code": 12
}

I have confirmed that

  • The FileEntry is constructed correctly (file to copy)
  • The DirectoryEntry is constructed correctly (copy destination)
  • The name is a string, and both success and failure callbacks are being passed in

So why is this failing? The source code for the copyTo method can be found here -- I'm stumped as to why this error is getting thrown, will return to the problem after lunch.

@the-bay-kay
Copy link
Contributor Author

Bingo -- here is my problem.

A copy of a file on top of an existing file must attempt to delete and replace that file.

Manually navigating to this folder and deleting the file results in a successful resolution. Woot! Now to test the share functionality in combination with this.

@the-bay-kay
Copy link
Contributor Author

Finally! Seems to be completely functional in the simulator. Going to run builds for iOS & Android -- if tests go OK, will move to R4R.

Screen.Recording.2024-06-26.at.2.mp4

- Added File I/O to send logs with `.txt` extension
- See PR 1160 for further details
@the-bay-kay
Copy link
Contributor Author

Good news: This is now working 100% on iOS! Adding the .txt extension solved our issues with GMail.

Bad news: As written, this PR does not work on Android. I'm almost certain this is due to the way the directory has been set up -- I think I made a mistake in the initial file path. Investigating now

@the-bay-kay
Copy link
Contributor Author

I'm almost certain this is due to the way the directory has been set up -- I think I made a mistake in the initial file path. Investigating now

Well, that wasn't the easy fix I thought it'd be -- time to break out my PC and run with .adb to see what's going on...

@louisg1337
Copy link
Contributor

Android Update

I was thankfully able to fix a few bugs on the Android side of things which, in conjunction with the rest of @the-bay-kay's work, now makes it function as expected! I ran into a few issues, which I'll touch on below, along with a caveat to my solution that I wanted to mention. PR can be found here.

Issues

  1. Android parentDirectory was slightly off

    The parentDirectory ended up being file:///data/user/0/edu.berkeley.eecs.emission/databases/, which we can represent as window['cordova'].file.dataDirectory.replace('files', 'databases'). To find this path, I learned that you can shell into a hardware device using adb shell, then "cd" into your app using run-as package-name, and from there you have access to all of the apps files.

  2. In the localCopyFile() function, fs.filesystem.root.getFile() could not find the loggerDB file

    Once I found the right directory we should be in, I was now getting a file not found error from getFile(). This was really strange because I was physically in the device and could confirm that loggerDB was in that directory. I then thought that maybe window['resolveLocalFileSystemURL'](parentDirectory, (fs => {})) was returning the wrong directory, but I took fs and created a DirectoryReader object, which let me scan the directory, and it returned loggerDB as one of the files in it. I did notice in the docs for getFile() that we can either pass in a relative path, which we do now, or an absolute path. When I pass in an absolute path, getFile() is able to find loggerDB and the rest of the functionality just works.

It is a bit annoying how I wasn't able to figure out why getFile() was not working with a relative path, but I am convinced it could maybe be an issue with the Android code and I don't know if its worth our time to debug, especially if we have the fix above. This solution also circumvents the original implementation a bit as we don't need to access the parentDirectory anymore on Android, but I don't think its too big of an issue.

@the-bay-kay
Copy link
Contributor Author

@louisg1337 Looks good to me! I'd maybe add a video showing the fix works, just so Shankari can merge this quick. Thanks again for finishing this off -- I had issues on iOS with relative file paths too, seems to be an issue when working with this plugin.

Before I forget -- I'll quickly update the JSON Dump feature, to fix it alongside this (no need for an extra PR, should be a < 2 line change!)

@louisg1337
Copy link
Contributor

Thanks for reviewing it for me @the-bay-kay! As request, here is the video of it work on Android...

screen-20240702-170055.mov

@the-bay-kay
Copy link
Contributor Author

@shankari, As written, this PR is finished and fixes the log functionality. Louis and I discussed fixing the JSON dump feature -- from an initial investigation, it appears that is broken for a different reason.

My question is: would you prefer we take the time and include the JSON Dump fix in this PR, or should we open a new PR for that? I would lean toward the latter, as this is a bottleneck for the iOS fixes (and is ready for review otherwise)

@shankari
Copy link
Contributor

shankari commented Jul 3, 2024

new PR. Also, it looks like we didn't add tests for the new code, so the code coverage check is failing...
I would be open to postponing the tests, but I want them in the very next PR that you submit

Copy link
Contributor

@shankari shankari left a comment

Choose a reason for hiding this comment

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

@louisg1337 @the-bay-kay please fix these issues ASAP so that I can merge before the weekend

www/i18n/en.json Outdated Show resolved Hide resolved
www/i18n/en.json Show resolved Hide resolved
www/js/services/shareLocalDBFile.ts Show resolved Hide resolved
www/js/services/shareLocalDBFile.ts Show resolved Hide resolved
www/js/services/shareLocalDBFile.ts Show resolved Hide resolved
www/js/services/shareLocalDBFile.ts Outdated Show resolved Hide resolved
www/js/services/shareLocalDBFile.ts Outdated Show resolved Hide resolved
www/js/services/shareLocalDBFile.ts Outdated Show resolved Hide resolved
www/js/services/shareLocalDBFile.ts Outdated Show resolved Hide resolved
Co-authored-by: K. Shankari <shankari@eecs.berkeley.edu>
@shankari shankari merged commit 3af54b7 into e-mission:master Jul 8, 2024
6 checks passed
shankari added a commit to shankari/e-mission-phone that referenced this pull request Sep 6, 2024
After the giant refactor, both "email log" and "download JSON dump" were
broken. We fixed "email log" in
e-mission#1160 but didn't fix "Download
JSON dump then, because of lack of time.

This fixes the "Download JSON dump" as well by making it similar to
implementation in "email log".

It was already fairly similar, but for some reason, was reading the data before
sharing the file

```
             const reader = new FileReader();

             reader.onloadend = () => {
             const readResult = this.result as string;
             logDebug(`Successfull file read with ${readResult.length} characters`);
```

and "this" doesn't exist, resulting in an undefined error.
Since the shareObj takes in a file name anyway, I just made this similar to the
emailLog changes by passing in the filename directly.

Also, similar ot the "Email Log", added a `.txt` extension so that the file can
be sent on iOS.

Testing done:
- Before this change: clicking on "Download JSON dump" did not do anything
- After this change: clicking on "Download JSON dump" launched the share menu

I haven't actually tried sharing yet because gmail is not configured on the emulator.
shankari added a commit to shankari/e-mission-phone that referenced this pull request Sep 7, 2024
After the giant refactor, both "email log" and "download JSON dump" were
broken. We fixed "email log" in
e-mission#1160 but didn't fix "Download
JSON dump then, because of lack of time.

This fixes the "Download JSON dump" as well by making it similar to
implementation in "email log".

It was already fairly similar, but for some reason, was reading the data before
sharing the file

```
             const reader = new FileReader();

             reader.onloadend = () => {
             const readResult = this.result as string;
             logDebug(`Successfull file read with ${readResult.length} characters`);
```

and "this" doesn't exist, resulting in an undefined error.
Since the shareObj takes in a file name anyway, I just made this similar to the
emailLog changes by passing in the filename directly.

Also, similar ot the "Email Log", added a `.txt` extension so that the file can
be sent on iOS.

Testing done:
- Before this change: clicking on "Download JSON dump" did not do anything
- After this change: clicking on "Download JSON dump" launched the share menu

I haven't actually tried sharing yet because gmail is not configured on the emulator.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants