Skip to content

Update grade report card rendering logic#70

Draft
Natan-gal wants to merge 10 commits into
SolDevelo:mainfrom
Natan-gal:patch-3
Draft

Update grade report card rendering logic#70
Natan-gal wants to merge 10 commits into
SolDevelo:mainfrom
Natan-gal:patch-3

Conversation

@Natan-gal
Copy link
Copy Markdown
Contributor

Changes

  • Hide grade cards for scanners that did not run
  • Hide Overall Grade when only one scanner is available
  • Prevent displaying misleading grades for skipped scans

Refactor grade report rendering to conditionally display grade cards based on violations.
@igor-soldev
Copy link
Copy Markdown
Member

Hi @Natan-gal,

Thanks for the Pull Request! Before we merge it, I have two requests:

  1. Link to the Issue
    Please update the PR description with the issue number it resolves (for example, by adding the word Closes followed by the issue number). This allows GitHub to automatically close the ticket.

  2. Fix the rendering conditions (Code Review)
    You mentioned in the description that you want to hide cards for scanners that did not run. However, the logic currently checks if the number of violations is greater than zero. This means that if a scanner runs and finds absolutely no issues, the card will be hidden. Zero violations is a perfect score that we definitely want to show to the user, not an indication that the scanner was skipped.

To fix this, instead of checking the number of violations, simply check if the report object for the specific scanner (cost, security, or container) actually exists. Please update this condition for all three scanners and let me know when you are done!

Fixed the scanner card rendering logic.

Previously, cards were hidden when the number of violations was 0, which also hid scanners that actually ran successfully with a perfect score.

Now the cards are rendered based on whether the scanner report object exists, so:

* scanners with 0 violations are still visible,
* scanners that did not run stay hidden,
* Overall Grade is only shown when more than one scanner result exists.

Closes SolDevelo#53
@Natan-gal
Copy link
Copy Markdown
Contributor Author

Updated the rendering conditions to check whether the scanner report object exists instead of checking the number of violations.

This now correctly displays scanners with 0 violations while still hiding scanners that did not run.

Closes #53

@igor-soldev
Copy link
Copy Markdown
Member

I tested the changes locally and noticed that when only the cost scanner is enabled, the Security and Container cards are still rendered with A / 100% results.

It looks like disabled scanners may still produce empty/default report objects, which makes the new existence-based rendering condition evaluate to true.

Could you verify whether skipped scanners should omit their report objects entirely instead of generating empty reports?

@Natan-gal
Copy link
Copy Markdown
Contributor Author

I've implemented the fixes for both issues:

Backend Fix (reporter/grading.py):

  • Modified generate_report() to only generate grade objects when the scanner actually ran with findings
  • Skipped scanners now return null instead of empty/default grades
  • Updated _calculate_overall_grade() and _generate_recommendations() to handle None grades

Frontend Fix (static/app.js):

  • Updated renderGradeReport() to check if report objects exist before rendering
  • Changed from checking violations count to checking if the grade object itself exists
  • This ensures:
    • Scanners with 0 findings show as A/100% (perfect score)
    • Skipped scanners show nothing (grade is null)

Both commits address your original feedback and the rendering condition issue. Ready for review!

@igor-soldev
Copy link
Copy Markdown
Member

I tried running the application locally with:

python3 app.py

but encountered an IndentationError in reporter/grading.py around line 303.

It looks like the issue is caused by incorrect indentation after one of the function definitions (generate_report() / _calculate_overall_grade() sections in the latest diff). Some blocks that should be nested inside the function body appear to be aligned with the function definition itself, which causes Python to fail during parsing.

Could you verify whether the application was tested locally after the latest changes? At the moment the app does not start on my side because of this indentation issue.

@igor-soldev igor-soldev marked this pull request as draft May 21, 2026 08:54
Copy link
Copy Markdown
Contributor Author

@Natan-gal Natan-gal left a comment

Choose a reason for hiding this comment

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

After testing the app locally, I fixed the issues related to single-scanner report handling and several NoneType crashes.

The updated logic now correctly handles partial scans (fast, security, containers) and the application works as expected during local testing.

Main fixes include:

  • safe handling for missing grade objects
  • fixes for .letter and severity_breakdown access on None
  • improved overall grade calculation for partial scans
  • corrected rendering for single-scanner results

@igor-soldev
Copy link
Copy Markdown
Member

Hi @Natan-gal,

Thanks for the updates and for testing this locally! The application no longer crashes with single-scanner runs, which is great.

However, during my review, I found a few issues that need to be addressed before we can merge this:

1. Broken HTML Layout in static/app.js (Critical for UI)

In the renderGradeReport function, you removed the opening <div class="grade-report-section"> container and the <h2 class="section-title">...</h2> header, but you left the closing </div> tag at the very bottom of the template string (around line 1434).
This creates an orphaned closing tag that will prematurely close a parent container in the DOM and break the page layout. Please either restore the opening tag (if it was removed by accident) or remove the trailing </div>.

2. Redundant Condition in reporter/grading.py

In _generate_recommendations, you have the following check:

elif all(
    g and g.letter == 'A'
    for g in [cost_grade, security_grade, container_grade]
    if g
) and total_findings > 0: 

Since the list comprehension already filters out falsy values with if g at the end, g is guaranteed to be truthy inside the loop. You can simplify this by removing the redundant g and:

elif all(
    g.letter == 'A'
    for g in [cost_grade, security_grade, container_grade]
    if g
) and total_findings > 0: 

3. Logic Bug in "Worst Grade" Calculation (Existed in the old code too!)

Just above that check, worst_grade is assigned like this:

worst_grade = min(available_letters) if available_letters else 'A'

In Python, strings are compared alphabetically (where 'A' < 'B' < 'C'). So, calling min() on ['A', 'F'] will return 'A'. This means the logic actually finds the best grade instead of the worst one, which prevents the infrastructure warnings for 'D' and 'F' grades from triggering properly!
Please change min to max so it correctly finds the letter furthest in the alphabet:

worst_grade = max(available_letters) if available_letters else 'A'

Once these are fixed, this should be good to go. Let me know if you have any questions!

Natan-gal added 3 commits May 25, 2026 17:32
* restored missing `grade-report-section` wrapper
* restored Infrastructure Health Report section title
* fixed orphaned closing </div> breaking DOM layout
* preserved single-scanner rendering logic
* ensured recommendations section renders correctly
* restored missing `grade-report-section` wrapper
* restored Infrastructure Health Report section title
* fixed orphaned closing </div> breaking DOM layout
* preserved single-scanner rendering logic
* ensured recommendations section renders correctly
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.

2 participants