✨ adding filter& sort by analysis to Application-inventory table #2100
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
HadasahR
requested review from
ibolton336,
sjd78 and
rszwajko
as code owners
HadasahR
changed the title
HadasahR
changed the title
HadasahR
changed the title
Signed-off-by: HadasahR <d025834828@gmail.com>
Signed-off-by: HadasahR <d025834828@gmail.com>
HadasahR
force-pushed
the
hadassah_application-inventoy-table-add-filtering-by-analysis
branch
from
0fa725a
to
c142659
Compare
HadasahR
changed the title
…ering-by-analysis
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2100 +/- ##
==========================================
+ Coverage 39.20% 42.01% +2.81%
==========================================
Files 146 175 +29
Lines 4857 5617 +760
Branches 1164 1392 +228
==========================================
+ Hits 1904 2360 +456
- Misses 2939 3136 +197
- Partials 14 121 +107
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
rszwajko
requested changes
Sep 26, 2024
There was a problem hiding this comment.
The code for new functionality is added in the correct places but there are some problems with mapping state values. Unfortunately adding code to legacy class of this size is never easy :)
General comments:
- wording: the PR title is OK (you could start with a capital letter though), the description is repeating the title so you could skip it. However leave a link to the issue/bug you are fixing i.e. Resolves: [BUG] Need to be able to filter & sort by application analysis status #1745 . In git history you will find some examples
- check out your IDE configuration - it seems the IDE is not auto-formatting on save action. We have a script for linting - run from command line (in the project root)
npm run lint. You will see the list of all violations (we somehow accumulated few of them). Then you can fix (only) yours. General cleanup is a candidate for another PR (npm run lint -- --fixwill auto-fix all easy formatting problems in the whole project)
| @@ -345,6 +348,7 @@ export const ApplicationsTable: React.FC = () => { | |||
| businessService: app.businessService?.name || "", | |||
| tags: app.tags?.length || 0, | |||
| effort: app.effort || 0, | |||
| analysis:app.tasks.currentAnalyzer?.state || "" | |||
There was a problem hiding this comment.
Sort values need to be the same as the values visible to the user. Otherwise the sort order may feel "random".
| what: t("terms.analysis").toLowerCase(), | ||
| }) + "...", | ||
| selectOptions: Array.from(taskStateToAnalyze).map( | ||
| ([taskState, displayStatus]) => ({ |
There was a problem hiding this comment.
Unfortunately few values are mapped to the same value (see the screenshot below). You could remove duplicates in few ways but using a library is probably the fastest - here's an example from our code base. Note also that the values get sorted before the display.
| selectOptions: Array.from(taskStateToAnalyze).map( | ||
| ([taskState, displayStatus]) => ({ | ||
| value: taskState, | ||
| label: t(`${displayStatus}`), |
There was a problem hiding this comment.
The displayed values should match the values that the user sees in the table. Here the things get a bit tricky.
Basically you need to repeat the same flow as done in the table by ApplicationAnalysisStatus component. There the received state is mapped via taskStateToAnalyze BUT later on inside IconedStatus
it's mapped again - the label value is the value that the user sees.
| label: t(`${displayStatus}`), | ||
| }) | ||
| ), | ||
| getItemValue: (item) => item?.tasks.currentAnalyzer?.state || "No Task", |
There was a problem hiding this comment.
The value here should match the value prop in the selectOptions list. If this would not work please let me know - there are more options.
|
Thanks, I'll check it out.
בתאריך יום ה׳, 26 בספט׳ 2024 ב-23:14 מאת Radoslaw Szwajkowski <
***@***.***>:
… ***@***.**** requested changes on this pull request.
The code for new functionality is added in the correct places but there
are some problems with mapping state values. Unfortunately adding code to
legacy class of this size is never easy :)
General comments:
1. wording: the PR title is OK (you could start with a capital letter
though), the description is repeating the title so you could skip it.
However leave a link to the issue/bug you are fixing i.e. Resolves:
#1745 <#1745> . In git
history you will find some examples
2. check out your IDE configuration - it seems the IDE is not
auto-formatting on save action. We have a script for linting - run from
command line (in the project root) npm run lint . You will see the
list of all violations (we somehow accumulated few of them). Then you can
fix (only) yours. General cleanup is a candidate for another PR (npm
run lint -- --fix will auto-fix all easy formatting problems in the
whole project)
------------------------------
In
client/src/app/pages/applications/applications-table/applications-table.tsx
<#2100 (comment)>:
> @@ -345,6 +348,7 @@ export const ApplicationsTable: React.FC = () => {
businessService: app.businessService?.name || "",
tags: app.tags?.length || 0,
effort: app.effort || 0,
+ analysis:app.tasks.currentAnalyzer?.state || ""
Sort values need to be the same as the values visible to the user.
Otherwise the sort order may feel "random".
------------------------------
In
client/src/app/pages/applications/applications-table/applications-table.tsx
<#2100 (comment)>:
> @@ -499,7 +503,24 @@ export const ApplicationsTable: React.FC = () => {
],
getItemValue: (item) => normalizeRisk(item.risk) ?? "",
},
+ {
+ categoryKey: "analysis",
+ title: t("terms.analysis"),
+ type: FilterType.multiselect,
+ placeholderText:
+ t("actions.filterBy", {
+ what: t("terms.analysis").toLowerCase(),
+ }) + "...",
+ selectOptions: Array.from(taskStateToAnalyze).map(
+ ([taskState, displayStatus]) => ({
Unfortunately few values are mapped to the same value (see the screenshot
below). You could remove duplicates in few ways but using a library is
probably the fastest - an example from our code base. Note also that the
values get sorted before the display.
Screenshot.from.2024-09-26.21-26-22.png (view on web)
<https://github.com/user-attachments/assets/d40772f2-8ff7-4684-8b33-d6cfaf63a9b5>
------------------------------
In
client/src/app/pages/applications/applications-table/applications-table.tsx
<#2100 (comment)>:
> @@ -499,7 +503,24 @@ export const ApplicationsTable: React.FC = () => {
],
getItemValue: (item) => normalizeRisk(item.risk) ?? "",
},
+ {
+ categoryKey: "analysis",
+ title: t("terms.analysis"),
+ type: FilterType.multiselect,
+ placeholderText:
+ t("actions.filterBy", {
+ what: t("terms.analysis").toLowerCase(),
+ }) + "...",
+ selectOptions: Array.from(taskStateToAnalyze).map(
+ ([taskState, displayStatus]) => ({
+ value: taskState,
+ label: t(`${displayStatus}`),
The displayed values should match the values that the user sees in the
table. Here the things get a bit tricky.
Basically you need to repeat the same flow as done in the table by ApplicationAnalysisStatus
component
<https://github.com/konveyor/tackle2-ui/blob/c04bb35390b3bd06e425dc8cead7cd92df2dd478/client/src/app/pages/applications/applications-table/applications-table.tsx#L929>.
There the received state is mapped via taskStateToAnalyze BUT later on
inside IconedStatus
<https://github.com/konveyor/tackle2-ui/blob/c04bb35390b3bd06e425dc8cead7cd92df2dd478/client/src/app/components/Icons/IconedStatus.tsx#L69>
it's mapped again - the label value is the value that the user sees.
------------------------------
In
client/src/app/pages/applications/applications-table/applications-table.tsx
<#2100 (comment)>:
> @@ -499,7 +503,24 @@ export const ApplicationsTable: React.FC = () => {
],
getItemValue: (item) => normalizeRisk(item.risk) ?? "",
},
+ {
+ categoryKey: "analysis",
+ title: t("terms.analysis"),
+ type: FilterType.multiselect,
+ placeholderText:
+ t("actions.filterBy", {
+ what: t("terms.analysis").toLowerCase(),
+ }) + "...",
+ selectOptions: Array.from(taskStateToAnalyze).map(
+ ([taskState, displayStatus]) => ({
+ value: taskState,
+ label: t(`${displayStatus}`),
+ })
+ ),
+ getItemValue: (item) => item?.tasks.currentAnalyzer?.state || "No Task",
The value here should match the value prop in the selectOptions list. If
this would not work please let me know - there are more options.
—
Reply to this email directly, view it on GitHub
<#2100 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/BFOSM5AKDARWL6MXDB3TKVLZYRTK7AVCNFSM6AAAAABONHNEEKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDGMZSGIYTOMJVGE>
.
You are receiving this because you modified the open/close state.Message
ID: ***@***.***>
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add filter&sort applications by analysis status #1745