✨ Fix missing archetype flow pieces #1368

ibolton336 · 2023-09-15T16:33:58Z

Closes #1298 #1278 #1332 #1324

Adjusts routes for archetype assessment flow vs application assessment flow
Creates a hook for looking up app or archetype based on their ID to avoid conditional hook calls.
Wire up create assessment override check

-Drive assessment wizard from assessment rather than questionnaire

With this PR, assessment flow is fully working for apps and archetypes against the hub outstanding PR.

codecov · 2023-09-15T16:41:09Z

Codecov Report

Patch coverage: 63.15% and project coverage change: +0.05% 🎉

Comparison is base (063e16a) 41.43% compared to head (666141c) 41.48%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1368      +/-   ##
==========================================
+ Coverage   41.43%   41.48%   +0.05%     
==========================================
  Files         137      137              
  Lines        4279     4288       +9     
  Branches     1026     1034       +8     
==========================================
+ Hits         1773     1779       +6     
- Misses       2418     2420       +2     
- Partials       88       89       +1

Flag	Coverage Δ
client	`41.48% <63.15%> (+0.05%)`	⬆️
server	`∅ <ø> (∅)`

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

Files Changed	Coverage Δ
client/src/app/api/models.ts	`100.00% <ø> (ø)`
client/src/app/queries/applications.ts	`47.54% <50.00%> (ø)`
client/src/app/queries/assessments.ts	`26.82% <50.00%> (ø)`
client/src/app/queries/reviews.ts	`20.75% <50.00%> (ø)`
client/src/app/components/ConfirmDialog.tsx	`75.00% <57.14%> (-15.00%)`	⬇️
...ents/custom-wizard-footer/custom-wizard-footer.tsx	`91.66% <75.00%> (+0.75%)`	⬆️
client/src/app/Paths.ts	`100.00% <100.00%> (ø)`

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

mturley

A couple of nitpicks here that may be a pain to address. I'm fine leaving them if you want, but wanted to express them anyway to see if you agree.

mturley · 2023-09-15T17:48:00Z

client/src/app/components/questionnaire-summary/questionnaire-summary.tsx

 }) => {
  const { t } = useTranslation();
-
+  const isArchetype = location.pathname.includes("/archetypes/");


Hm, what's the reason for changing this from a prop? If we ever needed to show the questionnaire content for an archetype outside the archetypes page (as a modal or something) this would be an issue. Not that we ever need to be able to do that necessarily, it's probably fine, just curious

I changed everything to be driven off of the location path because I was seeing some undefined props on hard navigation to some of the routes. Maybe that would be better handled by redirects. Some more testing is probably warranted here.

If you made the isArchetype prop required and provided a default value like isArchetype = false, then you would not see an undefined value.

Alternatively, you could use the useLocation or useRouteMatch hook from react-router to be a bit more robust in the isArchetype check.

client/src/app/hooks/useFetchArchetypeOrApplicationByAssessment.ts

mturley · 2023-09-15T17:51:22Z

client/src/app/pages/assessment/components/assessment-actions/assessment-actions-page.tsx


-  console.log("archetype", archetype);
+  console.log("archetype or app data", data);


Do we still need the console.log here?

client/src/app/pages/assessment/components/assessment-actions/assessment-actions-page.tsx

sjd78

Another way to organize the component would be to have an ApplicationAssessmentSummary and an ArchetypeAssessmentSummary. Then have those just wrap AssessmentSummary with a bunch of constants (isArchetype, title, urlPath1, urlPath2, etc). The child components would need to have access to those props (just passing them down is ok), but they wouldn't need to know how to select them.

For things like:

          <Text component="p">
            {isArchetype ? archetype?.name : application?.name}
          </Text>

The name can just come from a EntityWithName kind of type:

  ... // like in `items-select.tsx`:
  EntityWithName extends { name?: string };

  ...
  entityWithName: EntityWithName

  ...
          <Text component="p">{entityWithName?.name}</Text>

Then any archetype or application could be given as the entity.

None of that is a blocker though.

sjd78 · 2023-09-18T15:17:14Z

client/src/app/components/questionnaire-summary/questionnaire-summary.tsx

 }) => {
  const { t } = useTranslation();
-
+  const isArchetype = location.pathname.includes("/archetypes/");


If you made the isArchetype prop required and provided a default value like isArchetype = false, then you would not see an undefined value.

Alternatively, you could use the useLocation or useRouteMatch hook from react-router to be a bit more robust in the isArchetype check.

sjd78 · 2023-09-18T15:18:07Z

client/src/app/components/questionnaire-summary/questionnaire-summary.tsx

-          >
-            Assessment
-          </Link>
+          {isArchetype ? (


This could be push into the to prop instead of containing the full tag

sjd78 · 2023-09-18T15:22:28Z

client/src/app/pages/assessment/components/assessment-page-header.tsx

@@ -19,38 +20,38 @@ export const AssessmentPageHeader: React.FC<AssessmentPageHeaderProps> = ({
 }) => {
  const { t } = useTranslation();
  const history = useHistory();
+  const isArchetype = location.pathname.includes("/archetypes/");


This has been repeated enough times that it is a good candidate to create a hook for....

const isArchetype = useIsArchetypeFromLocation();

All good ideas. Will work on refactoring.

ibolton336 · 2023-09-18T18:56:58Z

Another way to organize the component would be to have an ApplicationAssessmentSummary and an ArchetypeAssessmentSummary. Then have those just wrap AssessmentSummary with a bunch of constants (isArchetype, title, urlPath1, urlPath2, etc). The child components would need to have access to those props (just passing them down is ok), but they wouldn't need to know how to select them.

For things like:
          <Text component="p">
            {isArchetype ? archetype?.name : application?.name}
          </Text>
The name can just come from a EntityWithName kind of type:
  ... // like in `items-select.tsx`:
  EntityWithName extends { name?: string };

  ...
  entityWithName: EntityWithName

  ...
          <Text component="p">{entityWithName?.name}</Text>
Then any archetype or application could be given as the entity.

None of that is a blocker though.

I like this pattern. I think it is a candidate for a refactor post feature freeze but am hesitant to attempt this with there being a third archetype/application agnostic usage for this summary component in the assessment-management-settings page.

mturley

One last nitpick here @ibolton336 . I do think there is some more room for cleanup here but I don't think it needs to block feature freeze. We should maybe open a tech debt issue to make sure we follow up.

mturley · 2023-09-19T19:08:28Z

client/src/app/hooks/useIsArchetype.ts

+  const [isArchetype, setIsArchetype] = useState(
+    location.pathname.includes("/archetypes/")
+  );
+
+  useEffect(() => {
+    setIsArchetype(location.pathname.includes("/archetypes/"));
+  }, [location.pathname]);


You don't need the state or the effect here because all they are doing is duplicating the new value from useLocation() when it causes a re-render. If that useEffect is being triggered, it means we already have an updated location.pathname value in scope (because useLocation watches for it to change and re-renders), and we can just return that directly instead of putting it in state. So I think this whole hook can be rewritten as:

const useIsArchetype = () => useLocation().pathname.includes("/archetypes/");

Updated - thanks!

sjd78

Just blocked on the return on the for loop, otherwise looks good.

client/src/app/components/ConfirmDialog.tsx

...pages/assessment/components/assessment-actions/components/dynamic-assessment-actions-row.tsx

Signed-off-by: ibolton336 <[email protected]> Drive isArchetype off of location in case of hard nav Signed-off-by: ibolton336 <[email protected]> Remove redundant query Signed-off-by: ibolton336 <[email protected]> Modify confirm dialog to include archetype assessment override alert Only show matching assessments for each entity type Update assessment override to use fetchassessment by archetype id approach wire up create assessment on confirm Use hook for isArchetype update test file Update logic for take assessment Wire discard assessment/review back in Fix hook and add override placeholder Signed-off-by: ibolton336 <[email protected]>

sjd78

LGTM

ibolton336 force-pushed the archetype-assessment-integration branch from 35629ae to 725e53d Compare September 15, 2023 16:34

ibolton336 marked this pull request as ready for review September 15, 2023 17:15

ibolton336 force-pushed the archetype-assessment-integration branch from d01d581 to fa5d61a Compare September 15, 2023 17:18

mturley reviewed Sep 15, 2023

View reviewed changes

ibolton336 force-pushed the archetype-assessment-integration branch from 8243742 to 94c4115 Compare September 15, 2023 20:55

ibolton336 requested a review from mturley September 15, 2023 20:55

sjd78 reviewed Sep 18, 2023

View reviewed changes

ibolton336 force-pushed the archetype-assessment-integration branch from 41a205f to 8632697 Compare September 19, 2023 14:40

mturley requested changes Sep 19, 2023

View reviewed changes

ibolton336 force-pushed the archetype-assessment-integration branch from cb8d2ed to 67ebb16 Compare September 19, 2023 19:29

ibolton336 requested a review from mturley September 19, 2023 19:30

sjd78 requested changes Sep 19, 2023

View reviewed changes

client/src/app/components/ConfirmDialog.tsx Outdated Show resolved Hide resolved

...pages/assessment/components/assessment-actions/components/dynamic-assessment-actions-row.tsx Outdated Show resolved Hide resolved

ibolton336 force-pushed the archetype-assessment-integration branch from 67ebb16 to 666141c Compare September 19, 2023 21:21

mturley approved these changes Sep 19, 2023

View reviewed changes

ibolton336 requested a review from sjd78 September 19, 2023 21:23

sjd78 approved these changes Sep 19, 2023

View reviewed changes

ibolton336 merged commit c36b614 into konveyor:main Sep 19, 2023
6 checks passed

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

✨ Fix missing archetype flow pieces #1368

✨ Fix missing archetype flow pieces #1368

ibolton336 commented Sep 15, 2023 •

edited

Loading

codecov bot commented Sep 15, 2023 •

edited

Loading

mturley left a comment

mturley Sep 15, 2023

ibolton336 Sep 15, 2023

sjd78 Sep 18, 2023

mturley Sep 15, 2023

sjd78 left a comment

sjd78 Sep 18, 2023

sjd78 Sep 18, 2023

sjd78 Sep 18, 2023

ibolton336 Sep 18, 2023

ibolton336 commented Sep 18, 2023 •

edited

Loading

mturley left a comment

mturley Sep 19, 2023

ibolton336 Sep 19, 2023

sjd78 left a comment

sjd78 left a comment


		console.log("archetype", archetype);
		console.log("archetype or app data", data);

✨ Fix missing archetype flow pieces #1368

✨ Fix missing archetype flow pieces #1368

Conversation

ibolton336 commented Sep 15, 2023 • edited Loading

codecov bot commented Sep 15, 2023 • edited Loading

Codecov Report

mturley 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

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

ibolton336 commented Sep 18, 2023 • edited Loading

mturley left a comment

Choose a reason for hiding this comment

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

sjd78 left a comment

Choose a reason for hiding this comment

ibolton336 commented Sep 15, 2023 •

edited

Loading

codecov bot commented Sep 15, 2023 •

edited

Loading

ibolton336 commented Sep 18, 2023 •

edited

Loading