-
Notifications
You must be signed in to change notification settings - Fork 39
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
✨ Initial Assessment page #1268
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #1268 +/- ##
==========================================
+ Coverage 43.11% 43.14% +0.03%
==========================================
Files 143 143
Lines 4291 4299 +8
Branches 1000 1000
==========================================
+ Hits 1850 1855 +5
- Misses 2361 2364 +3
Partials 80 80
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
Signed-off-by: Gilles Dubreuil <[email protected]>
Signed-off-by: Gilles Dubreuil <[email protected]>
Signed-off-by: Gilles Dubreuil <[email protected]>
Signed-off-by: Gilles Dubreuil <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, a couple small things.
@@ -704,3 +704,13 @@ export type HubFile = { | |||
name: string; | |||
path: string; | |||
}; | |||
|
|||
export interface Questionnaire { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we name this CustomQuestionnaire
to distinguish it from PathfinderQuestionnaire
rather than imply inheritance between the two? or is it intended that all questionnaires will eventually use this type and pathfinder objects will be deprecated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mturley, that's a good idea although my understanding is that Pathfinder objects are going to become obsolete.
Let me get confirmation that from the Hub team.
The other thing is the custom questionnaire is a subset of questionnaire because there are system questionnaire provided by Tackle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if pathfinder will be removed eventually and we'll only have one questionnaire type, i'm fine with leaving the name as-is.
to={Paths.questionnaires} | ||
activeClassName="pf-m-current" | ||
> | ||
Assessment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose this is design feedback for Justin but it seems confusing to me that we'll now have two views called Assessment - one here and one as a tab in application inventory. I guess because this is under admin it makes sense, but I wonder if we should propose that the nav item and page title are called "Questionnaires" instead, and a subtitle in the page header could be used to explain that they are for assessment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mturley,
My understanding is that the Administrator -> Assessment page is actually displaying the questionnaires but the Required button activate those questionnaires to the default Assessment process.
So the Questionnaires.tsx page might actually not the correct name. I wonder if I should rename it now or wait.
Because this page handles Assessment settings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah maybe Assessment Settings is a better name than Assessment then. I just find the current name potentially confusing to users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ibolton336, what do you think about renaming that page Assessment.tsx ?
Because the used case is about providing Administrator a way to control assessment, which starts with activating (Required column) questionnaires.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still find that to be a naming conflict. I'd rather it be something like AssessmentTools/QuestionaireSettings/AssessmentSettings etc.
Signed-off-by: Gilles Dubreuil <[email protected]>
Signed-off-by: Gilles Dubreuil <[email protected]>
Signed-off-by: Gilles Dubreuil <[email protected]>
Signed-off-by: Gilles Dubreuil <[email protected]>
The hub is going to provide Questionnaire type so the legacy Questionnaire (in model.ts) is renamed PathfinderQuestionnaire which is likely to become obsolete.
This uses PF5 Dropdown for Kebab menu instead of deprecated Dropdown.
Resolves #1260