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

Add Decision col to Pkg Dependency table #784

Merged
merged 14 commits into from
Jun 4, 2024
Merged

Add Decision col to Pkg Dependency table #784

merged 14 commits into from
Jun 4, 2024

Conversation

aclark02-arcus
Copy link
Collaborator

Seeks to close the first part of #774 with a start of adding the Decision column to the pkg dependency tables. Recall, there are three spots this table shows up: the pkg dep module, the report preview mod, and the reports themselves.

I think everything is kosher, and I even managed to enhance the "build report", which previously wasn't rendering the score and decision fields with color. It appears to be a little tricker to do this on the HTML reports, so I didn't tackle it in this PR.

Two pesky things I noticed in this PR is...

(1) that the menu won't disappear on the Build Report view:

image

(2) the console keeps showing this message when the pkg deps tab and build report tabs are opened:

image

@aclark02-arcus aclark02-arcus marked this pull request as ready for review June 3, 2024 15:08
@aclark02-arcus aclark02-arcus requested review from jthompson-arcus and removed request for jthompson-arcus June 3, 2024 15:13
Copy link
Collaborator

@jthompson-arcus jthompson-arcus left a comment

Choose a reason for hiding this comment

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

Couple things:

  1. For uploaded packages with no decisions, the app is currently displaying NA. I feel like this should be blank or maybe No Decision with a gray background. We could also use - like we do in the database view tab.
    image
  2. I don't see why decision_id is being pulled in. Am I missing something?

@aclark02-arcus
Copy link
Collaborator Author

aclark02-arcus commented Jun 3, 2024

2. I don't see why decision_id is being pulled in. Am I missing something?

decision_id will be needed for part II of this PR. I just didn't want to mess with that query twice. To summarize, I want to build a single card that communicates two new metrics for any given pkg:

  • the number of dependencies that are the highest decision category (presumably the highest risk), and...
  • the number of dependencies that are not the lowest category (presumably the non-low risk category)

I think both stats would be useful to know. So I need decision_id to do that. Do you have any comments or critiques on that approach? I think it works and I'll add a sentence here to coach deployers to know that they should list their categories from lowest risk to highest risk. I guess I'll add some special logic that if only two decision categories exist, than only the top statistic is needed.

@jthompson-arcus
Copy link
Collaborator

That makes sense to me. We have so much logic in the app that assumes an ordering to the decision categories anyways. I think it's natural to assume that even if an org wants additional categories for workflow purposes, those could be housed between the reject/accept decisions.

@aclark02-arcus
Copy link
Collaborator Author

@jthompson-arcus: updated!

image

Copy link
Collaborator

@jthompson-arcus jthompson-arcus left a comment

Choose a reason for hiding this comment

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

Couple more things:

  1. NAs are still showing up on the PDF reports for the decision column.
  2. The table on the build report/viewer tab is showing -1 for number of entries to view. (Not sure if your PR introduced this but if you could fix it that would be great.)
    image

@aclark02-arcus
Copy link
Collaborator Author

Fixed @jthompson-arcus! However, this is still showing up:

(2) the console keeps showing this message when the pkg deps tab and build report tabs are opened:

image

@jthompson-arcus jthompson-arcus requested review from jthompson-arcus and removed request for jthompson-arcus June 4, 2024 20:47
Copy link
Collaborator

@jthompson-arcus jthompson-arcus left a comment

Choose a reason for hiding this comment

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

Console warning is related to get_text_color() function. It is expecting a single value but getting a list. It also shows up when viewing the database viewer. I will push a fix in a separate PR.

@jthompson-arcus jthompson-arcus merged commit fdb0850 into dev Jun 4, 2024
1 check passed
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.

3 participants