Allow import of CSV into assumed PG instance #7
Conversation
pdobacz
force-pushed
the
piotr/type-preview-import-csv
branch
from
24ce1ba
to
6e427b4
Compare
I think that table import should be done in the main admin panel tab, which gets reset to the initial step at the end of the import (so not closed). |
yeah that makes sense too! Maybe best if I do it in a follow-up to not bloat the PR further. |
This seem very unlikely to happen. And even if it does, the attacker won't get the response, so no data leakage can occur. And our database will store a copy of the original data, so no loss of data can occur either. |
On the "unlikely" part - I don't know... this is for the open data initiative after all. If we succeed, we should expect anything to be thrown at this. And Of course, nothing to tackle right now, but the more I think about this, the more I think we should leave an issue behind to revisit. |
| "AidSelectionHelp": { | ||
| "ID Selection": "ID Selection", | ||
| "<strong>WARNING:</strong> If this configuration is not done correctly, the data will not be properly anonymized.": "<strong>WARNING:</strong> If this configuration is not done correctly, the data will not be properly anonymized.", | ||
| "If the data has one row per person (or other <1>protected entity</1>), then no entity identifier column need be selected. Otherwise, select a column containing a unique ID per protected entity. <4>Click here for details on how to set.</4>": "If the data has one row per person (or other <1>protected entity</1>), then no entity identifier column need be selected. Otherwise, select a column containing a unique ID per protected entity. <4>Click here for details on how to set.</4>" |
There was a problem hiding this comment.
I don't like at all the amount of duplication and noise the current translation approach creates.
This is especially annoying for long strings. I think we should start using some shorter tags for the keys, i.e. "AidConfigurationWarning".
There was a problem hiding this comment.
Agreed, but let's have a separate issue for this (also to handle uniformly in desktop, once we have the spare cycles)
| return true; | ||
| } catch (e) { | ||
| console.error(e); | ||
| message.error({ content: t('Data import failed!'), key: file.path, duration: 10 }); |
There was a problem hiding this comment.
I don't think it is a good idea for these notifications to have a limited duration.
If the import process takes even a small amount of time, it will be very likely that the user will change focus to another app and will come back later to check the result, missing the notification.
There was a problem hiding this comment.
Good point, let's have an issue and revisit later.
| const t = useT('AidSelectionStep'); | ||
| const [aidColumn, setAidColumn] = useState(''); | ||
| const [buttonState, setButtonState] = useState({ | ||
| title: `Import into ${file.name} (destructive operation!)`, |
There was a problem hiding this comment.
Why is this destructive? We should ask for confirmation in case the table already exists.
There was a problem hiding this comment.
:) all comments are good points for follow-ups. I'll just add the issues right away and we decide on when/how etc.
For now it is a desctructive operation, because it deletes the (automatically named) table and reimports. There are I suppose multiple ways to improve here, depending on how we provide the table name change facility.
| if (aidColumn == rowIndexColumn) { | ||
| await client.query(`ALTER TABLE "${tableName}" ADD COLUMN IF NOT EXISTS "${aidColumn}" SERIAL`); | ||
| } | ||
| await client.query(`CALL diffix.mark_personal('"${tableName}"', '"${aidColumn}"');`); |
There was a problem hiding this comment.
The user should be able to import public tables as well.
Closes #3. Comments:
connectionConfig- this is my own working setup, I didn't pay much attention to make it work for anythig else, also pending the lifecycle-management part. I guess putting your own values there should work