Modernize UI #798

[johnmhoran]

This PR revamps minimally the UI using the same overall look and feel
as ScanCode.io.

Further work beyond the basics including addressing the reviewers
feedback will take place in:
#847

Reference: #798
Signed-off-by: John M. Horan [email protected]

[pombredanne]

Here is some comments on the current state of this PR... The goal would be to have a minimally new UI working so we can release it in verision 30.0:

On the landing page

• remove template in navbar

JMH: Done -- hidden using debug setting in navbar (affects all 5 primary pages).

third-party JS

• Remove https://ajax.googleapis.com/ajax/libs/jquery/3.6.0/jquery.min.js https://github.com/nexB/vulnerablecode/blob/441852f29a3d43b5ccf898ebdb1c405cb6e402d5/vulnerabilities/templates/vulnerability.html#L515 we cannot source JS from third-party sites. We would instead keep a Jquery under version control https://github.com/nexB/scancode.io/tree/main/scancodeio/static

JMH: Done -- downloaded and added jquery-3.6.0.js and jquery-3.6.0.min.js to vulnerablecode/vulnerablecode/static/js/ and added <script type="text/javascript" src="{% static 'js/jquery-3.6.0.js' %}"></script> to the head section of base.html.

Package search

• Replace HTML by proper link in the packages search results list page
• In the package detail view hide everything after fixing vulnerabilities

JMH: Both are done.

Vulnerabilities search

• Replace HTML by proper link in the vulnerabilities search results list page

JMH: Done.

• In the vulnerabilities detail view hide everything after fixed package
  • hide everything after aliases on the essential tabs
  • Populate Affected Packages and Fixed Packages

JMH: Both are done.

for later:

• [ ] limit searches to a minimum number of characters See limit searches to a minimum number of characters #871

[pombredanne]

Here are a few more suggestions:

• for the package details, we could have no tabs and everything in one view

• for the vulnerabilities details, a more compact view that would also display affected and fixed packages.
• The first few affected packages and first few fixed packages would be always listed on the main tab.
• There would be a ... (dot dot dot) linking to the tab with the full table
  • It seems Bulma tabsets do not enable such indirect links to a tab, so I added the dots via a for loop and above added a parenthetical (see xxx tab)
• Full details would be in the tabs.
• The count of references, and other items would be added to the right of the tab title inside parenthesis as in References (23)

[johnmhoran]

• [ ] Fix the sorting for all lists in the UI. See Fix the sorting for all lists in UI, particularly version-based sorting #872
• Change documentation link in navbar from API doc to the ReadTheDocs.
• Remove dash in vulnerabilities reference list when there is no value in a field. Same in other fields.
• Make sure search box in both search results pages and both details pages are populated with current search/display value(s).
• Make sure footer appears at bottom of all "short" pages -- fix package details.
• [ ] Add info to package/purl details page reflecting closest version with no unresolved reported vulnerabilities. (Do we not want to use the term "reported"?) See Add info to purl details re closest subsequent version with no unresolved vulnerabilities #873
• [ ] Add tests for new UI views, templates etc. See Add tests for new UI views, templates etc. #874
• [ ] Allow lowercase searches for VULCOID-* (which might be renamed). See Allow lowercase searches for VULCOID-* #875
• Clean up custom.css.

[tdruez]

@johnmhoran See my various comments for code improvements.
In general, you are using too many "style" attributes that you could replace with existing CSS classes.
Refer to the Bulma docs, in particular https://bulma.io/documentation/helpers/spacing-helpers/

[tdruez]

The QS should live outside the try block.

[johnmhoran]

@tdruez I don't understand -- what's the reason to move the Qs outside the try block, and how would that be done?

[johnmhoran]

Perhaps you're suggesting that I move the entire return() into a new method, leaving only purl = PackageURL.from_string(package_name), which tests whether the input is a syntactically-correct purl? Pass purl to the new method, keeping the try focused on its sole purpose: the purl test?

[tdruez]

what's the reason to move the Qs outside the try block

In general, you want to keep only one piece of logic that could fail within a try block.
An exception raised by converting a PURL has nothing to do with an exception filtering a QuerySet.

Also, using generic except: is bad practice. You should always try to be explicit about the exception you want to catch. Since you want to "Check whether the input value is a syntactically-correct purl", let's catch that specific Exception:

>>> PackageURL.from_string('wrong syntax')
Traceback (most recent call last):
  File "<console>", line 1, in <module>
  File "/Volumes/nexB/repos/scancode.io/lib/python3.9/site-packages/packageurl/__init__.py", line 354, in from_string
    raise ValueError(
ValueError: purl is missing the required "pkg" scheme component: {repr(purl)}.

The packageurl raise a ValueError in that case, that's the exception we want to catch.
You code could become:

try:
    purl = PackageURL.from_string(package_name)
except ValueError:
    purl = None

if purl:
    packages = ...
else:
    packages = ...

return list(packages)

This way, this are properly grouped, the code is more readable:

1. we deal with the purl
2. we prepare a queryset based on the available data
3. we return the fetched objects

keeping the try focused on its sole purpose: the purl test?

Yes, bottom line is to always try to keep you try block as focused as possible.

[johnmhoran]

Far out. Thank you for the detailed explanation @tdruez . I'll tackle this in the A.M. 👍

[johnmhoran]

See new issue #876

[tdruez]

This should be moved to the models.py as a method on the Package QuerySet.

[johnmhoran]

@tdruez What's the reason to do this, and how would I implement?

[johnmhoran]

I've not yet had a chance to give this a careful look. If our schedule permits (we don't have much overlap, and it might be another day before I could followup again if need be), I'd prefer to do that first and give it a try, although my familiarity with Django models is still rather shallow.

Re the rationale -- I modelled def _related_packages(self) on def _package_vulnerabilities(self), which is just above it inside class PackageUpdate(UpdateView). Why does one of these belong in models.py as a method on the Package QuerySet while the other belongs in the view?

And another newb question: does adding it as a QuerySet method just mean adding it as a method to the model, e.g, def related_packages(self, package_url) -- no preceding _ -- and figuring out how to pass this to the view/template?

[tdruez]

What's the reason to do this, and how would I implement?

To keep thing properly organized, re-usable, and easier to test.

In a Django app, code related to the database transaction belongs in the models.

Why does one of these belong in models.py as a method on the Package QuerySet while the other belongs in the view?

I don't see any QuerySet logic in _package_vulnerabilities.

And another newb question: does adding it as a QuerySet method just mean adding it as a method to the model, e.g, def related_packages(self, package_url) -- no preceding _ -- and figuring out how to pass this to the view/template?

Not exactly, it means adding this on the QuerySet class associated with a model.

Some examples:
https://github.com/nexB/scancode.io/blob/main/scanpipe/models.py#L1215
https://github.com/nexB/scancode.io/blob/main/scanpipe/models.py#L960

The first step is to add a QuerySet class to your Package model:

class PackageQuerySet(models.QuerySet):    

You can then start to break your various filters and annotation into reusable methods on the PackageQuerySet class:

return list(
            models.Package.objects.all()
            .filter(
                Q(
                    type=purl.type,
                    namespace=purl.namespace,
                    name=purl.name,
                    subpath=purl.subpath,
                    qualifiers=purl.qualifiers,
                )
            )
            .order_by("version")
            .annotate(
                vulnerability_count=Count(
                    "vulnerabilities",
                    filter=Q(packagerelatedvulnerability__fix=False),
                ),
                # TODO: consider renaming to fixed in the future
                patched_vulnerability_count=Count(
                    "vulnerabilities",
                    filter=Q(packagerelatedvulnerability__fix=True),
                ),
            )
            .prefetch_related()
        )

Could become:

class PackageQuerySet(models.QuerySet): 
    def for_package_url(self, purl):
        return self.filter(
                type=purl.type,
                namespace=purl.namespace,
                name=purl.name,
                subpath=purl.subpath,
                qualifiers=purl.qualifiers,
        )

    def with_vulnerability_counts(self):
        return self.annotate(
                vulnerability_count=Count(
                    "vulnerabilities",
                    filter=Q(packagerelatedvulnerability__fix=False),
                ),
                patched_vulnerability_count=Count(
                    "vulnerabilities",
                    filter=Q(packagerelatedvulnerability__fix=True),
                ),
            )

Then on the view side, you have something much more readable such as:

return list(
    models.Package.objects
    .for_package_url(purl)
    .order_by("version")
    .with_vulnerability_counts()
    .prefetch_related()
)

Add this on your Package model before the "Meta" class and after all fields, such as https://github.com/nexB/scancode.io/blob/main/scanpipe/models.py#L1469

objects = PackageQuerySet.as_manager()

You now have the database logic organized in the models, you can re-use the count annotations without copy pasting the code into other views, and you view logic is more focused and readable.

Note: The code above is not tested, simply here to illustrate, it'll have to be adapted and refined.

[johnmhoran]

See new issue #880

[tdruez]

Use f-strings for better readability:

search_string = request.GET["name"]
"package_search": f"The VCIO DB does not contain a record of the package you entered: {search_string}."

[johnmhoran]

See new issue #877

[johnmhoran]

@tdruez Re my using too many style attributes, I'll do my best to replace these with Bulma classes as you requested, but it will take a fair amount of time to figure out what the Bulma version is of a specific display style I've fine-tuned, and not all likely are covered by the class choices provided by Bulma. Working on this now.

[tdruez]

Q object is not needed here, it is only required for complex query such as applying OR between filters such as https://github.com/nexB/vulnerablecode/pull/813/files#diff-3e8fc96d993d0bd8585642f68239732a30a19b7ec100a36ee5d21e209625ff81R202.
This comment applies to all the place you have a single Q in your filter().

[johnmhoran]

See new issue #878

[tdruez]

.all() is not needed if you call .filter() after.

[johnmhoran]

See new issue #879

[tdruez]

This is also a good candidate to move on the PackageQuerySet class:

def order_by_purl(self):
    self.order_by("type", "namespace", "name", "version", "subpath", "qualifiers")

Then on the view side:

models.Package.objects.[...].order_by_purl()

[johnmhoran]

See new issue #880

[tdruez]

• Fix the labels and wording case consistency