✨ Add Tasks tab to the Applications table application details drawer #2123

EstyBiton · 2024-10-08T12:19:53Z

Enhancing the application details drawer by adding a "Tasks" tab, displaying a table of tasks associated with the application, implementing filtering, sorting, and pagination functionalities, and including columns for Task ID, Task Kind, Status, and Priority.

Fixes: #1937

screenshot of result:

Signed-off-by: EstyBiton <[email protected]>

Signed-off-by: esty <[email protected]> Signed-off-by: EstyBiton <[email protected]>

Signed-off-by: EstyBiton <[email protected]>

codecov · 2024-10-29T17:57:20Z

Codecov Report

Attention: Patch coverage is 0% with 6 lines in your changes missing coverage. Please review.

Project coverage is 41.95%. Comparing base (b654645) to head (cdd77a7).
Report is 244 commits behind head on main.

Files with missing lines	Patch %	Lines
client/src/app/pages/tasks/useTaskActions.tsx	0.00%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2123      +/-   ##
==========================================
+ Coverage   39.20%   41.95%   +2.75%     
==========================================
  Files         146      175      +29     
  Lines        4857     5635     +778     
  Branches     1164     1397     +233     
==========================================
+ Hits         1904     2364     +460     
- Misses       2939     3150     +211     
- Partials       14      121     +107

Flag	Coverage Δ
client	`41.95% <0.00%> (+2.75%)`	⬆️
server	`?`

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

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

🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests
JS Bundle Analysis - Avoid shipping oversized bundles

sjd78

The work done is quite good. There are some things that need to be changed.

On the side bar table, either drop the priority column or make the initial width of the drawer larger. With the current width of the drawer, the action kebab are not visible. The other option is to make the drawer something like 100px wider initially so the task action kebab is shown.
When linking to the task details, the URL is breaking. You can follow the same format as navigateToAnalysisDetails(), but it should really be its own path.

The status column should use the same column style as the tasks-page (status icon, name, and link). See:

tackle2-ui/client/src/app/pages/tasks/tasks-page.tsx

Lines 244 to 257 in 0cfe60b

    
           state: ( 
        
             <IconWithLabel 
        
               icon={<TaskStateIcon state={state} />} 
        
               label={ 
        
                 <Link 
        
                   to={formatPath(Paths.taskDetails, { 
        
                     taskId: id, 
        
                   })} 
        
                 > 
        
                   {t(taskStateToLabel[state ?? "No task"])} 
        
                 </Link> 
        
               } 
        
             /> 
        
           ),

Need to fix the link that are being generated on task manager table (see specific comment).

I have a few more specific comment in the review.

sjd78 · 2024-10-30T19:02:44Z

client/src/app/Paths.ts

@@ -41,7 +41,7 @@ export const DevPaths = {

  dependencies: "/dependencies",
  tasks: "/tasks",
-  taskDetails: "/tasks/:taskId",
+  taskDetails: "/tasks/:taskId/:isFApplication",


This is causing a bad path on the Task Manager page:

sjd78 · 2024-10-30T20:37:38Z

...rc/app/pages/applications/components/application-detail-drawer/application-detail-drawer.tsx

+  const tableControls = useTableControlProps({
+    ...tableControlState,
+    idProperty: "id",
+    currentPageItems: currentPageItems,
+    totalItemCount,
+    isLoading: isFetching,
+    selectionState: useSelectionState({
+      items: currentPageItems,
+      isEqual: (a, b) => a.name === b.name,
+    }),
+  });


I think a compact table looks better here. Add prop variant: "compact", and see if you agree.

sjd78 · 2024-10-30T20:38:53Z

...rc/app/pages/applications/components/application-detail-drawer/application-detail-drawer.tsx

+                    modifier="nowrap"
+                    {...getTdProps({ columnKey: "status" })}
+                  >
+                    {task.state}


Make this render like on the task manager page -- status icon with a link to view the task details

sjd78 · 2024-10-30T20:47:41Z

...rc/app/pages/applications/components/application-detail-drawer/application-detail-drawer.tsx

+                  item={task}
+                  rowIndex={rowIndex}
+                >
+                  <Td width={10} {...getTdProps({ columnKey: "taskId" })}>


The Td widths are not necessary. All of the patterfly examples put any width values on the Th tags only. Since I don't see any difference with them here or not, easiest is to remove them.

sjd78 · 2024-10-30T21:09:12Z

client/src/app/pages/tasks/TaskDetails.tsx

+  const [applicationName, setApplicationName] = useState<string | undefined>();
+  const [applicationId, setApplicationId] = useState<number | undefined>();
+
+  useEffect(() => {


@rszwajko Can you have a look at these changes? I don't remember exactly how these components hang together.

I missed this comment. I will take a look.

Signed-off-by: EstyBiton <[email protected]>

EstyBiton · 2024-11-03T09:22:44Z

@sjd78 Thank you for your review! I’ve made the following updates based on your feedback:

Removed priority field and unnecessary widths.
Updated rendering: The table now renders like the task manager page, featuring a status icon with a link to view task details.
Changed to compact table: I added the variant="compact" prop to enhance the appearance of the table.
screenshot:

rszwajko

The PR looks pretty impressive! It basically adds the entire feature. We could merge it now however too keep the code DRY we need a follow-up refactorings.
It would be better to merge the main refactoring (extracting base table component) as a prerequisite (different PR), merge it and then use it here. Please let me know if you can work on this (details in the comments). If not we can consider other variants.

rszwajko · 2024-11-19T20:54:18Z

client/src/app/Paths.ts

@@ -108,4 +109,5 @@ export interface AnalysisDetailsAttachmentRoute {
 export interface TaskDetailsAttachmentRoute {
  taskId: string;
  attachmentId: string;
+  applicationId: string;


After this change the interface is the same as AnalysisDetailsAttachmentRoute (just above).

rszwajko · 2024-11-19T21:16:14Z

client/src/app/pages/tasks/TaskDetails.tsx

Note that after the changes the file duplicates the functionality of AnalysisDetails - in fact analysis details is just a task that is highlighted for user convenience. The code behind is/was generic - if you replace the IDs in the URL you should be able to view any task. Ideally you should be able to re-use this code after extracting to some "base" class - similar how both TaskDetails and AnalysisDetails use TaskDetailsBase. IMHO the refactoring (extracting base functionality) is small enough to do in this PR.

It could look the following

const AnalysisDetails = () => <GoodNameForBaseComponent formatDetailsPath={(applicationId,taskId) => formatPath(Paths.applicationsAnalysisDetails, { applicationId, taskId, }); formatTitle={(taskName) => `Analysis details for ${taskName}`} // whatever else is needed /> const ApplicationTaskDetails = () => <GoodNameForBaseComponent formatDetailsPath={(applicationId,taskId) => ...); formatTitle={(taskName) => ...} // whatever else is needed />

rszwajko · 2024-11-19T21:24:01Z

...rc/app/pages/applications/components/application-detail-drawer/application-detail-drawer.tsx

+  const tableControlState = useTableControlState({
+    tableName: "tasks-apps-table",
+    persistTo: "urlParams",
+    persistenceKeyPrefix: TablePersistenceKeyPrefix.tasks,


it seems we have a collision with TasksPage

rszwajko · 2024-11-19T21:34:12Z

...rc/app/pages/applications/components/application-detail-drawer/application-detail-drawer.tsx

@@ -521,3 +554,206 @@ const TabReportsContent: React.FC<{
    </>
  );
 };
+
+const TabTasksContent: React.FC<{


Ideally we should be able to reuse the code from TasksPage. In the current state this requires a refactoring that would extract the "base" component. The best strategy would be to use a different PR for that and merge it first. This way we (and our test suite) ensure that the refactoring caused no regression.

After the refactoring both TasksPage and TabTasksContent should use the base and provide the custom parts. There are few differences:

identifiers i.e. tableName, persistenceKeyPrefix

visible columns - note that hiding (via initialColumns) looks easier then maintaining 2 lists of columns

query params - implicitFilters

possibly more

If "base" component variant would be not achievable we can break the TasksPage into smaller parts and try to reuse those.

@rszwajko Thank you for the feedback! I would really like to continue working on this task, but I currently have a challenge. My internship with the company has concluded, and I no longer have access to the computer where the product environment was properly set up. Unfortunately, running Konveyor on Minikube in the Windows operating system presents a significant issue: the local Konveyor UI is inaccessible.
If the issue with installing Konveyor is resolved, I’d be happy to continue working on this task. Please let me know.

EstyBiton requested review from ibolton336, sjd78 and rszwajko as code owners October 8, 2024 12:19

EstyBiton marked this pull request as draft October 8, 2024 12:21

EstyBiton added 4 commits October 8, 2024 15:43

Add Tasks tab to the Applications table

c08d206

Signed-off-by: EstyBiton <[email protected]>

breadcrumb navigation back to the application table

7fc6cbb

Signed-off-by: esty <[email protected]> Signed-off-by: EstyBiton <[email protected]>

Update breadcrumbs for improved navigation

48565da

Signed-off-by: EstyBiton <[email protected]>

Add Tasks tab to the Applications table application details drawer

fc085f3

Signed-off-by: EstyBiton <[email protected]>

EstyBiton force-pushed the tasks-tab-drawer branch from ed6042b to fc085f3 Compare October 8, 2024 12:44

EstyBiton marked this pull request as ready for review October 9, 2024 06:37

sjd78 requested changes Oct 30, 2024

View reviewed changes

mguetta1 mentioned this pull request Oct 31, 2024

[Draft] Test tasks tab in detail drawer konveyor/tackle-ui-tests#1242

Open

Fix tasks tab in detail drawer

033323d

Signed-off-by: EstyBiton <[email protected]>

EstyBiton requested a review from sjd78 November 3, 2024 09:24

Merge branch 'main' into tasks-tab-drawer

cdd77a7

rszwajko reviewed Nov 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Add Tasks tab to the Applications table application details drawer #2123

✨ Add Tasks tab to the Applications table application details drawer #2123

EstyBiton commented Oct 8, 2024 •

edited by sjd78

Loading

codecov bot commented Oct 29, 2024 •

edited

Loading

sjd78 left a comment

sjd78 Oct 30, 2024

sjd78 Oct 30, 2024

sjd78 Oct 30, 2024

sjd78 Oct 30, 2024

sjd78 Oct 30, 2024

rszwajko Nov 19, 2024

EstyBiton commented Nov 3, 2024 •

edited

Loading

rszwajko left a comment

rszwajko Nov 19, 2024

rszwajko Nov 19, 2024

rszwajko Nov 19, 2024

rszwajko Nov 19, 2024

EstyBiton Nov 20, 2024

	state: (
	<IconWithLabel
	icon={<TaskStateIcon state={state} />}
	label={
	<Link
	to={formatPath(Paths.taskDetails, {
	taskId: id,
	})}
	>
	{t(taskStateToLabel[state ?? "No task"])}
	</Link>
	}
	/>
	),

✨ Add Tasks tab to the Applications table application details drawer #2123

Are you sure you want to change the base?

✨ Add Tasks tab to the Applications table application details drawer #2123

Conversation

EstyBiton commented Oct 8, 2024 • edited by sjd78 Loading

codecov bot commented Oct 29, 2024 • 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

EstyBiton commented Nov 3, 2024 • edited Loading

rszwajko 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

EstyBiton commented Oct 8, 2024 •

edited by sjd78

Loading

codecov bot commented Oct 29, 2024 •

edited

Loading

EstyBiton commented Nov 3, 2024 •

edited

Loading