Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Support xlsx upload for measurement metadata registration #803

Merged
merged 29 commits into from
Aug 28, 2024
Merged

Support xlsx upload for measurement metadata registration #803

merged 29 commits into from
Aug 28, 2024

Conversation

sven1103
Copy link
Contributor

Solves #677

@sven1103 sven1103 linked an issue Aug 22, 2024 that may be closed by this pull request
Support XLSX upload for measurement metadata registration / update #677
Closed
@sven1103 sven1103 marked this pull request as ready for review August 26, 2024 13:48
@sven1103 sven1103 requested a review from a team as a code owner August 26, 2024 13:48
KochTobi
KochTobi requested changes Aug 27, 2024
View reviewed changes
Copy link
Member

@KochTobi KochTobi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks good 👍 Great job.
Some minor remarks I would like you to address

user-interface/src/main/java/life/qbic/datamanager/parser/MetadataConverter.java Outdated Show resolved Hide resolved
user-interface/src/main/java/life/qbic/datamanager/parser/xlsx/TSVParser.java Outdated Show resolved Hide resolved
user-interface/src/main/java/life/qbic/datamanager/parser/xlsx/TSVParser.java Outdated Show resolved Hide resolved
user-interface/src/main/java/life/qbic/datamanager/parser/xlsx/TSVParser.java Outdated Show resolved Hide resolved
user-interface/src/main/java/life/qbic/datamanager/parser/xlsx/TSVParser.java Outdated Show resolved Hide resolved
user-interface/src/main/java/life/qbic/datamanager/parser/MetadataConverter.java Outdated
* @since 1.4.0
*/
static Optional<NGSMeasurementProperty> fromStringTrailingIgnored(String value) {
var trimmedValue = value.trim();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

String:trim will remove all trailing AND all leading whitespace. Is this intended here? From the method name I would expect the leading whitespace to not be removed and only the trailing to be handled.

user-interface/src/main/java/life/qbic/datamanager/parser/ParsingResult.java Outdated
*/
public record ParsingResult(Map<String, Integer> keys, List<List<String>> values) {

public Stream<List<String>> rows() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about public Stream<Row> rows()

user-interface/src/main/java/life/qbic/datamanager/parser/ParsingResult.java Outdated
return values.stream();
}

public Iterator<List<String>> iterator() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about public Iterator<Row> iterator()

user-interface/src/main/java/life/qbic/datamanager/parser/ParsingResult.java Outdated
return values.iterator();
}

public List<String> getRow(int row) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about public Row getRow(int rowIndex)

...fe/qbic/datamanager/views/projects/project/measurements/MeasurementMetadataUploadDialog.java Show resolved Hide resolved
@sven1103 sven1103 requested a review from KochTobi August 27, 2024 12:50
KochTobi
KochTobi requested changes Aug 27, 2024
View reviewed changes
Copy link
Member

@KochTobi KochTobi left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice adjustments. I like the switch to the Row record. Increases readability a lot.
One minor thing when readin cell values and one question regarding measurement editing.

Regarding measurement editing:
I followed the steps detailled in the documentation. However when I

  1. Download the measurement metadata
  2. Clear rows at the end by selecting them and pressing backspace on my keyboard
  3. Save the XLSX file
  4. Upload the edited XLSX file

Then I get the error that at least one sample id is missing. This happens for both XLSX and TXT (UTF-16 tab separated)

Screenshot 2024-08-27 at 17 45 38 image Screenshot 2024-08-27 at 17 37 17

user-interface/src/main/java/life/qbic/datamanager/parser/ParsingResult.java
return rows.get(rowIndex).values;
}

public record Row(List<String> values) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

love it

...fe/qbic/datamanager/views/projects/project/measurements/MeasurementMetadataUploadDialog.java Outdated
@@ -828,12 +526,12 @@ public UploadItemsDisplay(Upload upload) {
uploadSectionTitle.addClassName("section-title");

var saveYourFileInfo = new InfoBox().setInfoText(
"Please save your excel file as UTF-16 Unicode Text (*.txt) before uploading.")
"Please save your Excel file as UTF-16 Unicode Text (*.txt) before uploading.")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would argue that this is no longer the only way as you can upload xlsx as well now.

user-interface/src/main/java/life/qbic/datamanager/parser/xlsx/XLSXParser.java Outdated Show resolved Hide resolved
sven1103 and others added 5 commits August 28, 2024 07:50
@sven1103
Merge branch 'development' into feature/#677-support-xlsx-upload-for-…
c396e56 
…measurement-metadata-registration
@sven1103
Filter empty rows
1df0968
@sven1103
Merge branch 'feature/#677-support-xlsx-upload-for-measurement-metada…
34609de 
…ta-registration' of github.com:qbicsoftware/data-manager-app into feature/#677-support-xlsx-upload-for-measurement-metadata-registration
@sven1103
Provide new bundle
d1c9970
@sven1103
Apply change request
d6f5cea
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Aug 28, 2024

Quality Gate Passed Quality Gate passed

Issues
4 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@sven1103
Copy link
Contributor Author

Regarding measurement editing: I followed the steps detailled in the documentation. However when I

1. Download the measurement metadata
2. Clear rows at the end by selecting them and pressing backspace on my keyboard
3. Save the XLSX file
4. Upload the edited XLSX file

I was able to reproduce it and included another method for the Sanitizer to be able to filter out empty rows for both TSVParser and XLSXParser.

@sven1103 sven1103 requested a review from KochTobi August 28, 2024 06:29
KochTobi
KochTobi approved these changes Aug 28, 2024
View reviewed changes
Copy link
Member

@KochTobi KochTobi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great Job!

@sven1103 sven1103 merged commit 5de2e98 into development Aug 28, 2024
4 checks passed
@sven1103 sven1103 deleted the feature/#677-support-xlsx-upload-for-measurement-metadata-registration branch August 28, 2024 07:15
KochTobi pushed a commit that referenced this pull request Aug 28, 2024
@sven1103 @KochTobi
Support xlsx upload for measurement metadata registration (#803)
d037c4d 
Adds XLSX file upload support for measurement metadata registration.

Solves #677

Co-authored-by: KochTobi <tobias.koch@qbic.uni-tuebingen.de>
(cherry picked from commit 5de2e98)
@sven1103 sven1103 mentioned this pull request Aug 29, 2024
Merged
@KochTobi KochTobi mentioned this pull request Oct 21, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KochTobi KochTobi KochTobi approved these changes

Assignees
No one assigned
Labels
feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support XLSX upload for measurement metadata registration / update
2 participants
@sven1103 @KochTobi