-
Notifications
You must be signed in to change notification settings - Fork 13
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
2751 ability to export sqlite database on android #3001
2751 ability to export sqlite database on android #3001
Conversation
if (!inetAddress.isLoopbackAddress() && !inetAddress.isLinkLocalAddress() && inetAddress.isSiteLocalAddress()) { | ||
return inetAddress.getHostAddress(); | ||
} | ||
for (Enumeration<NetworkInterface> en = NetworkInterface.getNetworkInterfaces(); en.hasMoreElements();) { |
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.
This is just reformatting from my editor not sure why...
@@ -7,7 +7,6 @@ use anyhow::Context; | |||
use chrono::NaiveDateTime; | |||
use log::error; | |||
use mime_guess::mime; | |||
use openssl::error; |
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.
This unused import was annoying me, but is side track...
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.
thanks - I was going to take that out too!
Bundle size differenceComparing this PR to
|
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.
π Nice!
Nothing major but a couple of small things that I'd like changed, as noted.
Have tested and it's working nicely! one weirdness is that when you are running in client mode against a SQLite server, the downloaded database is the locally running one, i.e. it's empty.
I think that could be misleading - perhaps something to add to the documentation? It might still be useful to have the file downloadable? If it isn't, we could just disable in that scenario.
); | ||
} | ||
|
||
fn validate_request( |
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.
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.
oh. let me answer my own question.
#[cfg(not(feature = "postgres"))]
pub async fn get_database(
if you change to using an if statement instead, then that will fix the problem.
I'm running postgres and get the warning, because the pg version of get_database
does not use the validate fn.
@@ -7,7 +7,6 @@ use anyhow::Context; | |||
use chrono::NaiveDateTime; | |||
use log::error; | |||
use mime_guess::mime; | |||
use openssl::error; |
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.
thanks - I was going to take that out too!
component={ | ||
<> | ||
<LoadingButton | ||
disabled={databaseSettings?.databaseType !== DatabaseType.SqLite} |
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.
If the button is going to show, it would be helpful to know why the button is disabled for me.
Either helper text below the Download Database
heading, or a tooltip, or the preferred option of the PM/design team (according to #1912) - the button is enabled but alerts when you click it.
That latter option is something I think none of the developers want, btw. Some help text?
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.
Definitely a tricky area with differing opinions on how to handle it. I did try to put in a title but doesn't show for disabled buttons. Another option is to take out the check entirely, then you get a backend error if you are on postgres. I nearly did that originally. I personally like tooltips but they're not so great on android...
Anyway, I've found I can get tooltips working by adding span element in, I think that will do for now?
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.
yep - that's good for me, thanks!
Co-authored-by: Mark Prins <[email protected]>
Co-authored-by: Mark Prins <[email protected]>
Fixes #2751
π©π»βπ» What does this PR do?
Allows users with Server Administration Access to download the SQLite database, on Android or via the web.
On Android the file is zipped, making it smaller to share. 74MB zipped down to 14MB so seems like a decent saving there.
Download button shows as disabled if the server is running postgres.
π§ͺ How has/should this change been tested?
Would be good to have testing on Android and on Desktop.
I think there will be a bug on android it running as a client and the server is in SQLite mode. I don't think that's too big an issue though.
π Any notes for the reviewer?
Download performance (not android) is very slow. Maybe an actix bug or needs buffering or something? Could be a useful follow up issue unless someone has an idea how to improve it.
I'm using the refresh token cookie to validate the download request via the API.
I feel like it might be nicer to have the actual access token in a cookie to validate things like this.
We could also separate out the authentication logic into a middleware which would reduce the code duplication (e.g. cookie extraction logic is now in two places) - See: #3002
π Documentation
Documentation on how to export the database on android would be useful.