-
Notifications
You must be signed in to change notification settings - Fork 114
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
Conversation
- 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
Testing UpdatesWhen 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 My first theory as to why this occurs: Gmail prohibits the sending of certain files (link). Because the 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
Codecov ReportAttention: Patch coverage is
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
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Quick Updates:
|
@the-bay-kay 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 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:
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:
It seems the relative path we're constructing is not getting resolved:
Why would this happen? Trying to go through the plugin docs for an answer... |
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 |
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:
|
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)} `)
|
Attempting a different approach, utilizing the Cordova File Plugin's 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...
I have confirmed that
So why is this failing? The source code for the |
Bingo -- here is my problem.
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. |
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
Good news: This is now working 100% on iOS! Adding the 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 |
Well, that wasn't the easy fix I thought it'd be -- time to break out my PC and run with |
Android UpdateI 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
It is a bit annoying how I wasn't able to figure out why |
Android Log Email Fix
@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!) |
Thanks for reviewing it for me @the-bay-kay! As request, here is the video of it work on Android... screen-20240702-170055.mov |
@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) |
new PR. Also, it looks like we didn't add tests for the new code, so the code coverage check is failing... |
There was a problem hiding this 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
…o sharing functions
Addressed review comments
Renamed clear data to something more accurate
Co-authored-by: K. Shankari <[email protected]>
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.
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.
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 flexiblesocialSharing
plugin that is used incontrolHelper.ts
. To reflect these updates, the service was renamed, and the English i18next entries updated.Recent Changes:
services/
directoryemailService.ts
to now use sharePlugin, to match controlHelpershareLocalDBFile
package-cordovabuild.json
Remaining Tests