✨ Remove pathfinder and convert assessment wizard to use new api #1315

ibolton336 · 2023-08-25T20:33:55Z

Screen.Recording.2023-08-30.at.10.19.07.AM.mov

This pull request aims to start the development of the single-app application assessment flow. It includes the addition of a full flow for the assessment process against the new data model, updates to match the latest Go structs from the hub PR, removal of pathfinder-specific elements, creation of msw mocks to handle assessment initial post and patch operations, and a few other things.

Changes Made:

Single-App Application Assessment Flow: Added a complete flow for single-app application assessment. This flow includes handling initial posts and patch operations.
Model Updates: Updated the models to align with the latest Go structs from the hub PR.
MSW Mocks Creation: Created msw mocks to handle assessment initial post and patch requests.

TODO:

Comments Feature: Comments feature is currently commented out in the assessments flow.
Stakeholders & Stakeholder Groups: Identified that the code related to stakeholders and stakeholder groups is not yet added to the assessment struct on the API. Collaboration with the hub team is needed to incorporate this functionality, as assessments can have associated stakeholders.
Mock Endpoint Enhancement: Need to add support for /applications/:id/assessments when available
Tests Update: Need to evaluate existing tests & update to match the new data format for assessments.
Assessment Risks Component: Will need to wire this up depending on the future availability of the calculatedRisk assessment field from the API
Hub Derived Fields for Assessments: Hub-derived fields for calculated risk not yet available. These fields are planned to be calculated by the hub after the final assessment submission, facilitating categorization in the reports screen.
Old Report Cards Consideration: Although the components for report cards still exist in the repository, their usage in reports.tsx has been commented out.

codecov · 2023-08-30T14:31:21Z

Codecov Report

Patch coverage: 52.17% and project coverage change: -0.38% ⚠️

Comparison is base (5757dcd) 42.92% compared to head (a9282ba) 42.54%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1315      +/-   ##
==========================================
- Coverage   42.92%   42.54%   -0.38%     
==========================================
  Files         146      136      -10     
  Lines        4454     4207     -247     
  Branches     1040     1000      -40     
==========================================
- Hits         1912     1790     -122     
+ Misses       2530     2405     -125     
  Partials       12       12

Flag	Coverage Δ
client	`42.54% <52.17%> (-0.38%)`	⬇️
server	`∅ <ø> (∅)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
client/src/app/queries/applications.ts	`47.36% <ø> (+0.59%)`	⬆️
client/src/app/queries/assessments.ts	`19.51% <25.00%> (+1.65%)`	⬆️
client/src/app/api/rest.ts	`55.62% <54.54%> (+0.32%)`	⬆️
...hooks/useAssessApplication/useAssessApplication.ts	`90.00% <100.00%> (ø)`
...ep-nav-description/wizard-step-nav-description.tsx	`100.00% <100.00%> (ø)`
.../applications/application-assessment/form-utils.ts	`100.00% <100.00%> (ø)`

... and 11 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

sjd78

Ok that was a lot to look through, but lots of consistent object/name changes.

Overall my impressions are it looks good. Maybe a bit more thought will need to go in to handling multiple assessments for a single application (no matter where the assessments are sourced) in terms of showing application/assessment statuses. A single status icon may not be comprehensive enough in the table. Not sure on that.

client/src/app/api/rest.ts

sjd78 · 2023-08-31T15:28:43Z

client/src/app/api/rest.ts

+export const getAssessmentsPromise = (filters: {
+  applicationId?: number | string;
+}): Promise<Assessment[]> => {


WDYT about just using destructuring (also kinda a nit-pick):

Suggested change

export const getAssessmentsPromise = (filters: {

applicationId?: number | string;

}): Promise<Assessment[]> => {

export const getAssessmentsPromise = ({ applicationId }: {

applicationId?: number | string;

}): Promise<Assessment[]> => {

and then just using applicationId later?

Not that it is applicable here, but default values are pretty easy this way. For example:

export const getAssessments = ({ applicationId = 1, }: { applicationId?: number | string } = {}) => { const query: string[] = buildQuery({ applicationId }); return axios.get(`${ASSESSMENTS}?${query.join("&")}`); };

(the extra default param = {} takes care of calls like getAssessments() or getAssessments(null))

I think that is a good idea. All of this code is also going to be deleted as soon as the back end guys finish the /applications/:id/assessments endpoint. This query param will go away entirely.

client/src/app/queries/questionnaires.ts

sjd78 · 2023-08-31T16:10:47Z

...ssessment/components/application-assessment-page/application-assessment-page-header.test.tsx

-      </ApplicationAssessmentPageHeader>
-    );
-    expect(wrapper).toMatchSnapshot();
+  it.skip("Renders without crashing", () => {


Just a thought on the render tests --

We can reuse the MSW layer to provide simple applications/assessments/etc for the render tests. It would be a step towards a fully mocked static demo UI. For some basic background on MSW on node/jest: https://mswjs.io/docs/getting-started/integrate/node

I'd like to open an issue for this to circle back on.

...cation-assessment/components/application-assessment-wizard/application-assessment-wizard.tsx

sjd78 · 2023-08-31T16:17:04Z

...lication-assessment/components/assessment-stakeholders-form/assessment-stakeholders-form.tsx

@@ -39,11 +39,10 @@ export const AssessmentStakeholdersForm: React.FC = () => {
  const { setValue, control, formState } =
    useFormContext<ApplicationAssessmentWizardValues>();

-  const { stakeholders } = useFetchStakeholders();
+  // const { stakeholders } = useFetchStakeholders();


Note to self - stakeholder work is on the TODO list

Yeah we need the hub guys to add stakeholders/stakeholderGroups to the assessment struct.

client/src/app/pages/applications/assessment-actions/components/assessment-actions-table.tsx

sjd78 · 2023-08-31T16:33:02Z

...ages/applications/components/application-assessment-status/application-assessment-status.tsx

  const { t } = useTranslation();
+  const { assessment } = useFetchAssessmentByID(assessments?.[0]?.id || 0);


Limiting the application to a the single first assessment is temporary?

Will there be times where the status of multiple assessments for a single application will be available and shown?

If there are multiple, will the 0th assessment always be the most current one and that is the one to show the status on?

That is a great question re: how are we showing net questionnaire status with multiple potential active questionnaires/assessments.

I raised this with Justin & Jeff. Not sure if we can expect a hub status for applications / archetypes around status. Probably need to think about if we want to perform an operation to check if "some" of the assessments have an in progress status etc. I will open an issue around this.

#1324

Looks like currently there is an "assessed" bool on the application within the hub struct. So maybe we just keep it simple for now and use that.

client/src/app/pages/assessment-management/questionnaire/questionnaire-page.tsx

Signed-off-by: ibolton336 <ibolton@redhat.com>

sjd78

Looking good.

mturley

I'm a little late finishing my review but LGTM :)

ibolton336 force-pushed the convert-assessment-wizard branch 3 times, most recently from 48a8986 to 0072127 Compare August 30, 2023 14:08

ibolton336 marked this pull request as ready for review August 30, 2023 14:08

ibolton336 changed the title ~~[WIP] Remove pathfinder and convert assessment wizard to use new api~~ ✨ Remove pathfinder and convert assessment wizard to use new api Aug 30, 2023

ibolton336 force-pushed the convert-assessment-wizard branch from 01ca3db to d561af2 Compare August 30, 2023 14:15

mturley self-requested a review August 31, 2023 13:09

sjd78 reviewed Aug 31, 2023

View reviewed changes

ibolton336 force-pushed the convert-assessment-wizard branch from 31b62bf to 8775e40 Compare August 31, 2023 17:07

Create mock single app assessment flow from questionnaire

ebfe5e8

Signed-off-by: ibolton336 <ibolton@redhat.com>

ibolton336 force-pushed the convert-assessment-wizard branch from 8775e40 to 8f64628 Compare August 31, 2023 17:07

PR comments addressed

a9282ba

Signed-off-by: ibolton336 <ibolton@redhat.com>

ibolton336 force-pushed the convert-assessment-wizard branch from 8f64628 to a9282ba Compare August 31, 2023 17:08

sjd78 approved these changes Aug 31, 2023

View reviewed changes

ibolton336 merged commit e041fe3 into konveyor:main Aug 31, 2023
6 checks passed

ibolton336 deleted the convert-assessment-wizard branch August 31, 2023 18:30

mturley reviewed Aug 31, 2023

View reviewed changes

This was referenced Nov 22, 2023

operator konveyor-operator (0.3.0-beta.3) k8s-operatorhub/community-operators#3570

Merged

operator konveyor-operator (0.3.0-beta.3) redhat-openshift-ecosystem/community-operators-prod#3652

Merged

This was referenced Jan 23, 2024

operator konveyor-operator (0.3.0) k8s-operatorhub/community-operators#3793

Merged

operator konveyor-operator (0.3.0) redhat-openshift-ecosystem/community-operators-prod#3927

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Remove pathfinder and convert assessment wizard to use new api #1315

✨ Remove pathfinder and convert assessment wizard to use new api #1315

ibolton336 commented Aug 25, 2023 •

edited

Loading

codecov bot commented Aug 30, 2023 •

edited

Loading

sjd78 left a comment

sjd78 Aug 31, 2023

ibolton336 Aug 31, 2023

sjd78 Aug 31, 2023

ibolton336 Aug 31, 2023

sjd78 Aug 31, 2023

ibolton336 Aug 31, 2023

sjd78 Aug 31, 2023

ibolton336 Aug 31, 2023 •

edited

Loading

ibolton336 Aug 31, 2023

sjd78 left a comment

mturley left a comment

		const { t } = useTranslation();
		const { assessment } = useFetchAssessmentByID(assessments?.[0]?.id \|\| 0);

✨ Remove pathfinder and convert assessment wizard to use new api #1315

✨ Remove pathfinder and convert assessment wizard to use new api #1315

Conversation

ibolton336 commented Aug 25, 2023 • edited Loading

codecov bot commented Aug 30, 2023 • edited Loading

Codecov Report

sjd78 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibolton336 Aug 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjd78 left a comment

Choose a reason for hiding this comment

mturley left a comment

Choose a reason for hiding this comment

ibolton336 commented Aug 25, 2023 •

edited

Loading

codecov bot commented Aug 30, 2023 •

edited

Loading

ibolton336 Aug 31, 2023 •

edited

Loading