Open quinthar opened 2 weeks ago
Job added to Upwork: https://www.upwork.com/jobs/~0143be8cb9f6347710
Triggered auto assignment to Contributor-plus team member for initial proposal review - @ishpaul777 (External
)
Triggered auto assignment to @stephanieelliott (NewFeature
), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details. Please add this Feature request to a GH project, as outlined in the SO.
We need to save logs in downloads folder on android
Feature requests
We need to update the native files below:
In here, we can use the function RNFetchBlob.fs.dirs.DownloadDir
to first get the path of the download directory on android and then later use RNFetchBlob.fs.cp(path, destinationPath)
to download the file on android device, the updated code would look like:
RNFetchBlob.fs.exists(RNFetchBlob.fs.dirs.DownloadDir)
.then(() => {
i
const destinationPath = `${RNFetchBlob.fs.dirs.DownloadDir}/${newFileName}`;
RNFetchBlob.fs.cp(path, destinationPath)
.then(() => {
Show meaningful path of saved logs.
New change.
Since files are already there in public folders, we can just show a meaningful path.
Instead of using file.path
, just show /Downloads/${file.newFileName}
for Android and /New Expensify Dev/${file.newFileName}
for iOS.
Same changes are needed for profiling path.
Also, change the newFilePath
to use DownloadDirectoryPath
for Android. For iOS, we can use the existing DocumentDirectoryPath
.
We want to save client side logging and profile trace files to the /Downloads folder on Android (and whatever similarly accessible folder is generally accessible on iOS.
Downloads
folder (can be seen here), but the file path that we're showing is of the localFile
, which might seem misleading to users because they might think the file hasn't been saved to Downloads
.Downloads
, but only have a local file, can be seen herelocalFile
path (the one that's in private folder as discouraged in the OP).In here, set the path
returned after saving the file to Downloads
to a mediaPath
field:
.then(path => {
setFile({
...localFile,
mediaPath: path,
});
});
Then in here, use the file.mediaPath
to show in path:
, if file.mediaPath
does not exist then fallback to file.path
(Since the device-download path looks cryptic, we maybe should instead show to the users the readable file name instead so they know how to access the file, something like path: /Downloads/logs-2024-04-14 10_58_29.432.txt
, this can easily be done by using the newFileName
)
Downloads
(or just to Files
), similar to what we did with Android, and also use the same mediaPath
approach too.NA
New feature to save app logs to Downloads instead of default app private folder.
This is a new feature.
Before suggesting my solution, @quinthar / @ishpaul777 I would like to bring 2 points to think about and consider before proceeding with the solution.
I propose to have some or all of these logs centralized and collected to one of the cloud service provider data stores location so that the same can be used for big data analytics and even for supporting in troubleshooting purposes. This must give a great insight into how the user is using the application and will also give insight into how we can improve our application and its user interface to provide better service to the user. Of course, this essentially means that we must get user consent (maybe as an optional service) at the time of installation and have these uploaded to data stores at regular intervals. Points to consider:
Having a log available in Downloads
folder means that the same can be accessed or read by others or by other applications. Having the logs saved into the app private folder structure is the best solution according to me.
@quinthar / @ishpaul777 : If my above thoughts make sense, I would like to work on this from backend as well as front end (from a full stack capacity) to architect, design and implement the solution.
N.A.
@brkdavis - Good questions! We already centrally aggregate these logs. They are just inconvenient to access, and only by a limited set of people (given all the concerns you raised). The point of local log capture is to make someone's own logs much easier for them to access. By capturing local logs into the Download folder, we make someone's own logs easier for them to access for whatever purpose they want. So, good questions, but I still would like to go ahead with this issue as stated.
@brkdavis - Good questions! We already centrally aggregate these logs. They are just inconvenient to access, and only by a limited set of people (given all the concerns you raised). The point of local log capture is to make someone's own logs much easier for them to access. By capturing local logs into the Download folder, we make someone's own logs easier for them to access for whatever purpose they want. So, good questions, but I still would like to go ahead with this issue as stated.
@quinthar : thanks a lot for the clarifications. Couple of questions in direction of logs in Downloads
folder.
Downloads
folder ss and when the log entries and generated? Or is it only upon 4 finger tap we must export the logs from private folder to Downloads
so that user can share from there at any point in time? I believe it is the latter. Kindly confirm.Do we have a log viewer within the app?
Yes, in About > Troubleshooting > View console logs
Should the logs automatically get logged to Downloads folder ss and when the log entries and generated? Or is it only upon 4 finger tap we must export the logs from private folder to Downloads so that user can share from there at any point in time? I believe it is the latter. Kindly confirm.
This issue is just about changing where it's logged, not when. So it's currently logged when you enable via 4-finger tap, or via About > Troubleshooting
. So let's keep it working as is, just change where it saves the logs to.
What are the next steps here; are we waiting on @ishpaul777 to pick a proposal? What's the ETA?
Hey @ishpaul777 we have a couple proposals here -- can you please review ASAP?
Hey Thanks for bump i have this planned for today : )
Started android build to test existing proposals, i'll test and update in a while
Thanks for your proposals everyone!
For Android, we're already saving to Downloads folder (can be seen here), but the file path that we're showing is of the localFile, which might seem misleading to users because they might think the file hasn't been saved to Downloads. -> from @dominictb Proposal
This seems correct we are just not showing the correct path but we do save logs in /downloads
folder but not the profile traces as we can see here
so first we need to fix it to save in downloads then we need to fix path shown for profile trace to downloads path
I'd like to see a proposal that fixes this
For iOS, we're not saving the file to Downloads, but only have a local file, can be seen here
i dont this we need fix the destination for ios, i think we already save it in a accessible folder i.e On my iphone -> New expensify, @quinthar Do you think we need to change this one?
Also as part of proposal please include how you plan to clean the path shown to user as most time its cryptic
As of now, on Android file is getting saved to internal directory and then being copied to Downloads. My proposal directly saves it to Downloads, because of which the path also shows correctly.
Contributor details Your Expensify account email: krispv@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/~01063aad97ce0e22c6
⚠️ Unable to store contributor details.
As of now, on Android file is getting saved to internal directory and then being copied to Downloads. My proposal directly saves it to Downloads, because of which the path also shows correctly.
I beleive the correct approach here is to just show the correct path instead of changing the whole approach
But isn't it redundant to first download it in one folder and then copy to another? We can improve performance by directly downloading to Downloads folder. Also, logs can become huge, so wastage of space to have it in two places.
But isn't it redundant to first download it in one folder and then copy to another? We can improve performance by directly downloading to Downloads folder. Also, logs can become huge, so wastage of space to have it in two places.
For Android, we might have to check for permission before saving (because of scoped storage). We can use the hasAndroidPermission method from fileDownload/index.android.ts.
i feel this is the reason we use this approach in first place so dont have to ask for extra permission, this was done quite intentionally https://github.com/Expensify/App/pull/38803/files#r1537727645, let me confirm from PR author asked here https://github.com/Expensify/App/pull/38803/files#r1571237179
@ShridharGoel Do you have branch for your solution i'll test it again meanwhile
Why do we need a seprate function we can just use existing one with a param "saveToDownloads" and when true we save to downloads folder also why LegacyDownloadDir
?
Yes, same function can be used with an extra parameter.
LegacyDownloadsDir
points to the common Downloads folder.
Okay so after a bit of research i found this issue https://github.com/RonRadtke/react-native-blob-util/issues/294, so it looks like LegacyDownloadsDir
uses depriciated apis under the hood and its not a future proof solution to use it, so i prefer to stick with current approach
Updated to just show a meaningful path.
@ShridharGoel and @dominictb For profile trace tool in android, file is not saved in downloads folder so proposals are incomplete, Please include changes required to save it in downloads folder, Thanks!
@ishpaul777 I had updated the infoFilePath while testing, missed it in the proposal. Updated now.
@ShridharGoel We want to save the logs in Downloads folder in first place, not when user clicks share
Save client side logging and profile trace files to the /Downloads folder on Android
@ishpaul777 Mixed up the names by mistake. I meant the newFilePath in rename method.
The copy task is being done in that method when the path changes (after profiling is stopped).
Proposal from @ShridharGoel Looks good to me!
Instead of using file.path, just show /Downloads/${file.newFileName} for Android and /New Expensify Dev/${file.newFileName} for iOS.
i'll let assigned engineer make the decision if we want to hardcode the path or just show the path as it is even if its kind of cryptic. (see screenshot)
🎀 👀 🎀 C+ reviewed
Triggered auto assignment to @techievivek, see https://stackoverflow.com/c/expensify/questions/7972 for more details.
📣 @ishpaul777 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!
📣 @ShridharGoel 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!
Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑💻 Keep in mind: Code of Conduct | Contributing 📖
@ishpaul777 @ShridharGoel I think we can hardcode the path name here as that will be easy to follow.
Thanks for clarifying @techievivek..
@ShridharGoel Can you please update what's your ETA for a PR
@ishpaul777 Will open the PR tomorrow.
Triggered auto assignment to @abekkala (NewFeature
), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details. Please add this Feature request to a GH project, as outlined in the SO.
Reapplying the New Feature
label to get another BZ member on this while I am OOO til May 2. Thanks @abekkala -- we're just waiting on a PR from @ShridharGoel. I'll grab this back from you when I return!
Problem:
We have a great system in place to enable client-side logging and profile tracing with a four-finger tap. This allows for really fantastic debugging on production devices out in the real world. However, the files are saved in a private folder that -- so far as I can tell -- is impossible for a user on a non-rooted device to access. So after getting the logs and profile traces, if they don't share them at that exact moment, there's no way to get them in the future.
Solution:
Save client side logging and profile trace files to the
/Downloads
folder on Android (and whatever similarly accessible folder is generally accessible on iOS.Upwork Automation - Do Not Edit