"Export to CSV" functionality for main tables in oVirt Administration Portal #901
Conversation
sandrobonazzola
force-pushed
the
csv-export
branch
from
f4aad03 to
b1207e0
Compare
...dules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/table/TableCsvExporter.java
Show resolved
Hide resolved
sermakov-orion
force-pushed
the
csv-export
branch
from
b1207e0 to
0a9eb1a
Compare
sandrobonazzola
force-pushed
the
csv-export
branch
from
0a9eb1a to
561b46e
Compare
sandrobonazzola
requested review from
BrooklynDewolf and
dupondje
and removed request for
ljelinkova
sandrobonazzola
force-pushed
the
csv-export
branch
from
561b46e to
02aff0a
Compare
sandrobonazzola
force-pushed
the
csv-export
branch
from
02aff0a to
9fb20fd
Compare
|
@dupondje @BrooklynDewolf can you please review? |
sandrobonazzola
force-pushed
the
csv-export
branch
from
9fb20fd to
db53b8f
Compare
sandrobonazzola
force-pushed
the
csv-export
branch
2 times, most recently
from
b0c1897 to
aeba0b2
Compare
… Portal Signed-off-by: Stepan Ermakov <[email protected]> This feature adds new menu item "Export to CSV" for the following tables of oVirt Administration Portal Compute * Virtual Machines * Templates * Pools * Hosts * Data Centers Network * Networks Storage * Domains * Volumes * Disks Events The menu item allows to export current content (taking in to account current localization, columns visibility, sort order, filter) of the selected table into CSV. And initiates automatic download of the created CSV file. Known limitation: not more than 10,000 can be exported.
sermakov-orion
force-pushed
the
csv-export
branch
from
aeba0b2 to
ea6f3d3
Compare
JasperB-TeamBlue
left a comment
JasperB-TeamBlue
left a comment
There was a problem hiding this comment.
Thanks for your commitment but there are some remarks about your PR:
- What is the decision process behind which tables are valid to export and which aren't?
- Exporting an empty CSV when there is nothing present seems wrong, it would be better if the button is disabled when there is nothing present
- Best practice would be to push the CSV generatiion code more to the backend, now it resides in the frontend, a place where it shouldn't be. An added benefit of moving this would be that an api call could be implemented that would have the same functionality as your current UI button.
| var pom = document.createElement('a'); | ||
| pom.setAttribute('href', 'data:text/plain;charset=utf-8,' + encodeURIComponent(text)); | ||
| pom.setAttribute('download', filename); | ||
| document.body.appendChild(pom); | ||
| pom.click(); | ||
| document.body.removeChild(pom); }-*/; |
There was a problem hiding this comment.
This is a commented out function so it does not actually do anything at the moment, please refactor this.
| // Note that in order to export content of the table we need to scroll to the first page, then export the content | ||
| // by moving forward page by page till the end (or till the 10000 rows limit is reached). And then return to the | ||
| // page where the export functionality was initiated. |
There was a problem hiding this comment.
Do I understand this correctly that you are reading out the data that you want to push to the csv from what is presented in the UI? Would it not be better practice to trigger an appropriate database call that could provide the needed data instead?
| * Table row data type. | ||
| **/ | ||
| public class TableCsvExporter<T> { | ||
| private static final int LINES_LIMIT = 10000; |
There was a problem hiding this comment.
Instead of hard coding variable in here would it not be better to save this in the database and link it to the ConfigValues, an example of this:
int timeout = Config.<Integer> getValue(ConfigValues.SetupNetworksWaitTimeoutSeconds)
|
|
||
| @Override | ||
| public String csvExportFilenameBase() { | ||
| return "events"; //$NON-NLS-1$ |
There was a problem hiding this comment.
Shouldn't there be a localized string used here? This way we ensure that the file name is in a language the end user actually understands, and if the language does not exist it can default to the english string.
| private static final String DOUBLE_QUOTE_STR = "\""; //$NON-NLS-1$ | ||
| private static final String DOUBLE_DOUBLE_QUOTE_STR = "\"\""; //$NON-NLS-1$ | ||
| private static final String FILE_EXT = ".csv"; //$NON-NLS-1$ | ||
| private static final String FILE_CURRENT_DATE_AND_TIME_FORMAT = "yyyy-MM-dd.HH-mm"; //$NON-NLS-1$ |
There was a problem hiding this comment.
This is not needed, there is a whole class dedicated to rendering time called FullDateTimeRenderer and you also have functions like DateTimeFormat.getFormat(DateTimeFormat.PredefinedFormat.DATE_TIME_MEDIUM);
4 participants
"Export to CSV" functionality for main tables in oVirt Administration Portal
Signed-off-by: Stepan Ermakov [email protected]
Changes introduced with this PR
This feature adds new menu item "Export to CSV" for the following tables of oVirt Administration Portal:
Compute
Network
Storage
Events
The menu item allows to export current content (taking in to account current localization, columns visibility, sort order, filter) of the selected table into CSV. And initiates automatic download of the created CSV file.
Known limitation: not more than 10,000 rows can be exported.
vms.2023-12-05.09-07.csv
Are you the owner of the code you are sending in, or do you have permission of the owner?
y