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.

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

Todo flags, Checkbox & Filter for reviewing licenses, Auto-scroll to focus bug fixes #610

Merged
merged 21 commits into from
Dec 6, 2023

Conversation

OmkarPh
Copy link
Collaborator

@OmkarPh OmkarPh commented Oct 22, 2023

Note-
This branch is a change on existing sqlite structure, so don't try importing existing sqlite files (from previous branches)

The behavior for recent files is as follows:

In (latest) production builds-

  • if you click on previously imported sqlite, it'll open the sqlite file directly
  • if you click on previously imported JSON, then also the sqlite file that was generated last time will be opened (Preserving vetted/unvetted license status)

In dev mode -

  • if you click on Open for a previously imported Sqlite file, it'll open the sqlite file and vetted feature would work fine.
  • if you import a JSON file and come back and click on import, the JSON file is re-parsed (clearing vetted/unvetted licenses status) This behavior is due to the fact that, during development, we tend to change the schema of sqlite & want to use latest schema to be used for parsing.
    To test vet/unvet feature in dev mode, first, import a JSON. & when coming back, import the sqlite file instead of using the history item. After this point, you'll see sqlite entry in history items, which you can import again and again & the vetted status will work just fine!

I'd also suggest, doing npm run publish and directly testing the final build

@OmkarPh OmkarPh changed the base branch from feature/unittests to update/docs October 22, 2023 08:13
@OmkarPh OmkarPh linked an issue Oct 22, 2023 that may be closed by this pull request
@OmkarPh OmkarPh changed the title Checkbox & Filter for vetted/unvetted licenses status Todo flags, Checkbox & Filter for vetted/unvetted licenses status Oct 23, 2023
@OmkarPh OmkarPh linked an issue Oct 23, 2023 that may be closed by this pull request
Base automatically changed from update/docs to feature/unittests October 25, 2023 13:58
Base automatically changed from feature/unittests to v4.0-react-typescript October 25, 2023 14:09
@OmkarPh OmkarPh changed the base branch from v4.0-react-typescript to develop October 25, 2023 18:30
@OmkarPh OmkarPh force-pushed the feature/license-review-status branch from ee8970a to 5c24602 Compare November 7, 2023 15:53
@OmkarPh OmkarPh changed the title Todo flags, Checkbox & Filter for vetted/unvetted licenses status Todo flags, Checkbox & Filter for reviewing licenses, Auto-scroll to focus bug fixes Nov 12, 2023
@OmkarPh OmkarPh mentioned this pull request Nov 12, 2023
Copy link
Member

@AyanSinhaMahapatra AyanSinhaMahapatra left a comment

Choose a reason for hiding this comment

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

Thanks++ @OmkarPh
See comments below


Couple of points on the License Explorer UI update:

  1. We have a new bug where we have a URL link highlighted for a spdx-license-identifier rule which was not the case before
  2. The matches table should be before the File regions table
  3. We can probably use the space better in the matches table with having two columns side-by-side for the attributes other than license-expression and matched-text
  4. We should have more info from the rule attributes in the matches table: like rule_length and the boolean flags too?

The Review feature and the checkboxes work perfectly

review-workbench


For the new Package Explorer -> License View navigation feature:

Now this looks something like this:

License Detections:
mpl-2.0 mpl-2.0
  1. I think the seperate license detections should be in different lines or in some other way which is more clear that the license detections are seperate entries
  2. For the scan linked below one of the link does not seem to work, goes to the License Explorer page with No License detection / clue selected

libzmq-4.3.5-scan-results.json.txt


I think it would be a good idea to create seperate PRs for seperate issues in future as it's easier to review, one feature will not get delayed by reviews on others, and more. We can always merge from main/base on different branches to get around the issues from this.

@OmkarPh
Copy link
Collaborator Author

OmkarPh commented Dec 4, 2023

Hey @AyanSinhaMahapatra
I've fixed the reported issues and implemented the suggestions
Rule details dialog is also ready to review

Copy link
Member

@AyanSinhaMahapatra AyanSinhaMahapatra left a comment

Choose a reason for hiding this comment

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

@OmkarPh A last couple of nits for your consideration, looks good otherwise.

  1. In licenses explorer we have a help text for the checkboxes: Tick the checkboxes below to mark licenses as reviewed, here licenses should be license detections
  2. The rule details dialog looks great, but we should have the i Info icon beside this to nudge the users more explicitly towards the dialog on hover, otherwise this is hidden.
  3. In some cases the SPDX license expression is missing. See for example the LicenseDetection with expression lgpl-2.0-plus AND lgpl-2.1-plus AND mpl-2.0 which has an issue, marked with an exclamation mark (in the attached scan). This should be resolved from the references properly. But note that in future we will be providing SPDX license expressions everywhere we now have a license-expression, to make it easier for end-users, this update will be added very soon.
    libzmq-4.3.5-scan-results.json.txt
    Thanks++ for the updates.

@OmkarPh
Copy link
Collaborator Author

OmkarPh commented Dec 5, 2023

  1. In licenses explorer we have a help text for the checkboxes: Tick the checkboxes below to mark licenses as reviewed, here licenses should be license detections

Also, I was wondering if we should duplicate this beside the license clues section below ?

  1. The rule details dialog looks great, but we should have the i Info icon beside this to nudge the users more explicitly towards the dialog on hover, otherwise this is hidden.

I didn't understand what you mean here, we already have it beside it right

  1. In some cases the SPDX license expression is missing. See for example the LicenseDetection with expression lgpl-2.0-plus AND lgpl-2.1-plus AND mpl-2.0 which has an issue, marked with an exclamation mark (in the attached scan). This should be resolved from the references properly.
    libzmq-4.3.5-scan-results.json.txt

Thanks for highlighting, fixed it

Copy link
Member

@AyanSinhaMahapatra AyanSinhaMahapatra left a comment

Choose a reason for hiding this comment

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

LGTM!
Thanks++ @OmkarPh , please merge and release 😄

@AyanSinhaMahapatra
Copy link
Member

Also, I was wondering if we should duplicate this beside the license clues section below ?

Not required probably, your call.

I didn't understand what you mean here, we already have it beside it right

Yeah it's visible now, not sure why it wasn't earlier, my bad. Thanks!

Thanks for highlighting, fixed it

Btw, there are still some issues for license-expressions that have WITH in the example above, see gpl-3.0-plus WITH autoconf-macro-exception

@OmkarPh
Copy link
Collaborator Author

OmkarPh commented Dec 6, 2023

Thansk @AyanSinhaMahapatra

Btw, there are still some issues for license-expressions that have WITH in the example above, see gpl-3.0-plus WITH autoconf-macro-exception

Yeah, my bad. I'll create another PR for this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support --todo option for reviews Add ability to track progress/status of license detection review
2 participants