chore(licenseinfo): Reporting Improvements Part 2 #433 #433

dreh23 · 2019-01-09T23:21:20Z

Development Detail table
Additional Requirement table

Signed-off-by: Johannes Amorosa johannes.amorosa@endocode.com

maierthomas

Does it make sense to store only the compId in LicenseInfoParsingResult and get all relevant information/data like swplatform, language... in the docxGernator instead of saving these data directly to the LicenseInfoParisingResult?

backend/src/src-licenseinfo/src/main/java/org/eclipse/sw360/licenseinfo/LicenseInfoHandler.java

mcjaeger · 2019-01-15T12:54:00Z

libraries/lib-datahandler/src/main/thrift/licenseinfo.thrift

@@ -70,6 +70,10 @@ struct LicenseInfoParsingResult {
    31: optional string name,
    32: optional string version,
    33: optional string componentType,
+
+    42: optional set<string> componentLanguages,


maybe the point here, agreeing with @maierthomas is that we have this not as part of the backend service API for the license info service - it appears rather as information that is processed internally to generate the document. Thus I also propose to pull this off from the API of the license inform service.

@dreh23 please have a look

mcjaeger · 2019-02-11T18:36:42Z

tested, works, but is conflicting
(also there seem to be a problem with SPDX files)

mcjaeger · 2019-02-22T16:14:35Z

@dreh23 I wondered in testing where is the setting for platform in the UI - was not able to test it

mcjaeger

Handling collection values seems to work with one value but not with multiple values.

mcjaeger · 2019-02-28T07:18:19Z

@dreh23 seems to be conflicting, maybe because of the merge of the previous pull request PR#438

mcjaeger · 2019-03-08T13:13:00Z

@dreh23 please try to resolve the conflicts

mcjaeger · 2019-03-12T09:13:08Z

@dreh23 signed commit?

mcjaeger

tested and looks OK

mcjaeger · 2019-02-22T16:22:37Z

libraries/lib-datahandler/src/main/thrift/licenseinfo.thrift

@@ -70,6 +70,10 @@ struct LicenseInfoParsingResult {
    31: optional string name,
    32: optional string version,
    33: optional string componentType,
+
+    42: optional set<string> componentLanguages,


@dreh23 please have a look

imaykay · 2019-03-21T11:14:12Z

Do we have any issue ticket that explains what the feature idea behind this PR is? I mean, tests seem to be already successful, but for code review it also interesting what should be achieved - at least for me.

/cc @mcjaeger @dreh23 @hemarkus

imaykay · 2019-03-21T14:10:44Z

...-licenseinfo/src/main/java/org/eclipse/sw360/licenseinfo/outputGenerators/DocxGenerator.java

@@ -151,6 +157,8 @@ private void fillReportDocument(
            String projectDescription = project.getDescription();

            fillOverview3rdPartyComponentTable(document, projectLicenseInfoResults);
+            fillDevelopmentDetailsTable(document, project, user);


If this table is placed before the 3rd party table in the document then I think it would be nice to see this in the structure of the code.

imaykay · 2019-03-21T14:14:04Z

...-licenseinfo/src/main/java/org/eclipse/sw360/licenseinfo/outputGenerators/DocxGenerator.java

+            row.addNewTableCell().setText(langs);
+            LOGGER.info(String.format("langs: %s", langs));
+
+            String platforms = component.getSoftwarePlatforms().isEmpty() ? "Unknown platforms" : String.join(" ", component.getSoftwarePlatforms());


At least with my data, getSoftwarePlatforms() has been NULL, so I could not generate the file because of a NPE. So maybe all these isEmpty() checks should have a null check first.

Even though I could not reproduce this I see the chance to hit a null reference due to Thirft. I added a check before accessing it.

imaykay · 2019-03-21T14:27:01Z

...-licenseinfo/src/main/java/org/eclipse/sw360/licenseinfo/outputGenerators/DocxGenerator.java

@@ -169,10 +177,14 @@ private void fillReportDocument(

            fillSpecialOSSRisksTable(document, project, obligationResults);

+            fillReleaseBulletList(document, projectLicenseInfoResults);
+            fillReleaseDetailList(document, projectLicenseInfoResults, includeObligations);
+            fillAdditionalRequirementsTable(document, obligationResults);


I know, there order was also not correct before your change, but maybe you could clean this up a bit? Maybe not only the order of the method calls but also the order of the methods in the class?

Actually those where not even supposed to be called here. Removed.

imaykay · 2019-03-21T14:29:25Z

...-licenseinfo/src/main/java/org/eclipse/sw360/licenseinfo/outputGenerators/DocxGenerator.java

+            row.addNewTableCell().setText(key.topic);
+            row.addNewTableCell().setText(licensesString);
+            row.addNewTableCell().setText(key.text);
+        }


I have no clue about the meanings of the different sections, but would some divider be of help here because the release bullet list just comes next?

dreh23 · 2019-03-22T12:07:41Z

@imaykay There are tickets https://github.com/siemens/sw360/projects/1 and there is a reference documentation that was provided which describes some aspects of the features. If @mcjaeger agrees I can share this doc with you.

On a side note I am not any more involved in the development of sw360. Please contact @hemarkus for any further questions.

imaykay · 2019-03-29T09:46:43Z

Code looks better now, but the latest changes do change a few things in the generated document. So maybe @mcjaeger, you want to retest if the result is still what you want it to be.

@hemarkus it would be nice if you could squash your commits into a single one like it is mentioned here https://github.com/eclipse/sw360/wiki/Dev-DoD-and-Style in section "Commit style".

Add Development Detail table and Additional Requirement table to the report document. Also order the calls to reflect the acutal documemt structure in code. Additionally refactor/fix nested loops. Signed-off-by: Markus Herpich <markus@endocode.com>

dreh23 changed the title ~~Added Reporting features:~~ chore(licenseinfo): Reporting Improvements Part 2 #433 Jan 10, 2019

mcjaeger added needs code review needs general test This is general testing, meaning that there is no org specific issue to check for labels Jan 10, 2019

maierthomas self-requested a review January 15, 2019 10:16

maierthomas reviewed Jan 15, 2019

View reviewed changes

backend/src/src-licenseinfo/src/main/java/org/eclipse/sw360/licenseinfo/LicenseInfoHandler.java Outdated Show resolved Hide resolved

backend/src/src-licenseinfo/src/main/java/org/eclipse/sw360/licenseinfo/LicenseInfoHandler.java Outdated Show resolved Hide resolved

maierthomas removed the needs code review label Jan 15, 2019

mcjaeger reviewed Jan 15, 2019

View reviewed changes

dreh23 force-pushed the johannesamorosa/ReportingPart2 branch from 057a07f to de7de6f Compare January 15, 2019 13:48

mcjaeger assigned mcjaeger and maierthomas Jan 21, 2019

mcjaeger added the has merge conflicts label Feb 11, 2019

dreh23 force-pushed the johannesamorosa/ReportingPart2 branch from 770609e to de7de6f Compare February 21, 2019 21:03

mcjaeger self-requested a review February 22, 2019 16:17

mcjaeger requested changes Feb 22, 2019

View reviewed changes

mcjaeger added WIP work in progress and removed has merge conflicts labels Feb 25, 2019

This was referenced Feb 25, 2019

Refactor pull request #433 Refactor data structure siemens/sw360#14

Open

Merge Conflicts on PR #433 siemens/sw360#15

Open

Fix multiple value bug on PR #433 siemens/sw360#16

Open

mcjaeger added the has merge conflicts label Feb 28, 2019

hemarkus force-pushed the johannesamorosa/ReportingPart2 branch 2 times, most recently from b198376 to 169463d Compare March 14, 2019 07:23

mcjaeger added WIP work in progress and removed WIP work in progress has merge conflicts labels Mar 14, 2019

mcjaeger added needs code review and removed WIP work in progress needs general test This is general testing, meaning that there is no org specific issue to check for labels Mar 14, 2019

mcjaeger self-requested a review March 14, 2019 14:08

mcjaeger approved these changes Mar 14, 2019

View reviewed changes

imaykay assigned imaykay and unassigned maierthomas Mar 21, 2019

imaykay suggested changes Mar 21, 2019

View reviewed changes

imaykay added WIP work in progress and removed needs code review labels Mar 21, 2019

imaykay removed their assignment Mar 21, 2019

imaykay approved these changes Mar 29, 2019

View reviewed changes

hemarkus force-pushed the johannesamorosa/ReportingPart2 branch from 5d041b2 to 67975c2 Compare March 29, 2019 10:31

mcjaeger merged commit b207789 into eclipse-sw360:master Mar 29, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(licenseinfo): Reporting Improvements Part 2 #433 #433

chore(licenseinfo): Reporting Improvements Part 2 #433 #433

dreh23 commented Jan 9, 2019

maierthomas left a comment

mcjaeger Jan 15, 2019

mcjaeger Feb 22, 2019

mcjaeger commented Feb 11, 2019

mcjaeger commented Feb 22, 2019

mcjaeger left a comment

mcjaeger commented Feb 28, 2019

mcjaeger commented Mar 8, 2019

mcjaeger commented Mar 12, 2019

mcjaeger left a comment

mcjaeger Feb 22, 2019

imaykay commented Mar 21, 2019

imaykay Mar 21, 2019

hemarkus Mar 26, 2019

imaykay Mar 21, 2019

hemarkus Mar 26, 2019

imaykay Mar 21, 2019

hemarkus Mar 26, 2019

imaykay Mar 21, 2019

hemarkus Mar 26, 2019

dreh23 commented Mar 22, 2019

imaykay commented Mar 29, 2019

chore(licenseinfo): Reporting Improvements Part 2 #433 #433

chore(licenseinfo): Reporting Improvements Part 2 #433 #433

Conversation

dreh23 commented Jan 9, 2019

maierthomas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcjaeger commented Feb 11, 2019

mcjaeger commented Feb 22, 2019

mcjaeger left a comment

Choose a reason for hiding this comment

mcjaeger commented Feb 28, 2019

mcjaeger commented Mar 8, 2019

mcjaeger commented Mar 12, 2019

mcjaeger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imaykay commented Mar 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dreh23 commented Mar 22, 2019

imaykay commented Mar 29, 2019