[kie-issues#1720] Enhance Process Details UI page to show nodeInstanceId each timer belongs to #2820

josedee · 2024-12-18T06:58:25Z

Currently we do not have node instance details for every job in the Process Details UI page. Hence adding nodeInstanceId corresponding to each job/timer.

josedee · 2024-12-18T07:10:21Z

Attaching few screenshots

Fixed below bugs

Added nodeInstanceId as shown below

Fixed timer icon and tooltip not appearing for all nodes

jomarko

Thank you for the pull request. I have a question about isNil usage. Could you please explain little bit more why we need it for:

job.priority
job.repeatInterval
job.repeatLimit

while we do not need it for:

job.scheduleId
job.callbackEndpoint

usually our effort is to minimize the need of used libraries. So just doublechecking if we need to introduce a dependency for lodash.isNil in this file.

jomarko · 2024-12-18T08:02:01Z

packages/runtime-tools-process-gateway-api/src/graphql/types.tsx

-/*
- * Licensed to the Apache Software Foundation (ASF) under one
- * or more contributor license agreements.  See the NOTICE file
- * distributed with this work for additional information
- * regarding copyright ownership.  The ASF licenses this file
- * to you under the Apache License, Version 2.0 (the
- * "License"); you may not use this file except in compliance
- * with the License.  You may obtain a copy of the License at
- *
- *  http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing,
- * software distributed under the License is distributed on an
- * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
- * KIND, either express or implied.  See the License for the
- * specific language governing permissions and limitations
- * under the License.
- */
-


Please undo this change, it is an important aspect of releasing Apache open source. Because of this change "CI :: License headers" check will fail.

@jomarko good catch, this file is autogenerated during codegen and codegen tool doesn't add the headers... @josedee please add the license back manually.

josedee · 2024-12-18T08:28:46Z

Thank you for the pull request. I have a question about isNil usage. Could you please explain little bit more why we need it for:

job.priority

job.repeatInterval

job.repeatLimit

while we do not need it for:

job.scheduleId

job.callbackEndpoint

usually our effort is to minimize the need of used libraries. So just doublechecking if we need to introduce a dependency for lodash.isNil in this file.

As seen in this screenshot, priority, repeatInterval and repeatLimit weren't displayed correctly.

In this example priority was either null/undefined, repeatInterval and repeatLimit are boths 0's. All three are numbers. I did this to display them when they are also 0. callbackEndpoint and scheduledId are both strings hence not requiring stringent validation

pefernan

+1 when licenses are back and all is green.

jomarko · 2024-12-18T09:31:14Z

@pefernan @josedee ok, that changes the situation, and I am sorry, it is my fault. If packages/runtime-tools-process-gateway-api/src/graphql/types.tsx is autogenerated, then the correct approach is to remove the license header from it, and add an entry into .rat-exludes file

like:

# packages/runtime-tools-process-gateway-api/src/graphql/types.tsx
types.tsx

josedee · 2024-12-18T11:11:54Z

@pefernan @josedee ok, that changes the situation, and I am sorry, it is my fault. If packages/runtime-tools-process-gateway-api/src/graphql/types.tsx is autogenerated, then the correct approach is to remove the license header from it, and add an entry into .rat-exludes file

like:
# packages/runtime-tools-process-gateway-api/src/graphql/types.tsx
types.tsx

@jomarko Does it consider all types.tsx files? Should we add only the specific tsx file. In this case

# packages/runtime-tools-process-gateway-api/src/graphql/types.tsx
packages/runtime-tools-process-gateway-api/src/graphql/types.tsx

jomarko · 2024-12-18T11:17:17Z

@josedee thank you for comment. It is currently not possible to add packages/runtime-tools-process-gateway-api/src/graphql/types.tsx, because we are blocked by apache/incubator-kie-issues#1670.

I see currently only two types.tsx files in the kie-tools codebase. If both are autogenerated, we can remove the license header from both and we should be fine adding types.tsx into .rat-excludes. What do you think?

ljmotta

Hi @josedee . Nice PR! I've just made some request changes inline regarding the lodash/isNil usage.

ljmotta · 2024-12-18T11:26:36Z

...oped-components/src/jobsManagement/envelope/components/JobsDetailsModal/JobsDetailsModal.tsx

@@ -26,6 +26,7 @@ import Moment from "react-moment";
 import "../styles.css";
 import { Job } from "@kie-tools/runtime-tools-process-gateway-api/dist/types";
 import { OUIAProps, componentOuiaProps } from "@kie-tools/runtime-tools-components/dist/ouiaTools";
+import isNil from "lodash/isNil";


Please, use the inline check (<value> === undefined or <value> !== undefined) instead of using a library to do so. This method checks if the value is null or undefined [1], and looking in the Job type, we don't have one case that can have those two values.

[1] https://lodash.com/docs#isNil

ljmotta · 2024-12-18T11:27:49Z

...oped-components/src/jobsManagement/envelope/components/JobsDetailsModal/JobsDetailsModal.tsx

              </Split>
            </FlexItem>
-            {job.repeatInterval && (
+            {!isNil(job.priority) && (


job.priority is from type number, you don't need to check if it will be null or undefined.

Thanks for the suggestions @ljmotta. I think we can use === or !== but one thing I think in the bug screenshot attached job.priority is displayed as Priority: without applying any validations. In that case I think it maybe null/undefined

ljmotta · 2024-12-18T11:28:13Z

...oped-components/src/jobsManagement/envelope/components/JobsDetailsModal/JobsDetailsModal.tsx

+                </Split>
+              </FlexItem>
+            )}
+            {!isNil(job.repeatInterval) && (


job.repeatInterval is from type number as well.

ljmotta · 2024-12-18T11:28:28Z

...oped-components/src/jobsManagement/envelope/components/JobsDetailsModal/JobsDetailsModal.tsx

@@ -93,7 +104,7 @@ export const JobsDetailsModal: React.FC<IOwnProps & OUIAProps> = ({
                </Split>
              </FlexItem>
            )}
-            {job.repeatLimit && (
+            {!isNil(job.repeatLimit) && (


...ocessDetails/envelope/components/ProcessDetailsTimelinePanel/ProcessDetailsTimelinePanel.tsx

pefernan · 2024-12-18T14:42:41Z

@ljmotta @josedee I agree on not using the isNill (although I proposed it originally), but I still think that we need that check because the graphql schema defines those properties as optional, so I'd prefer having it just in case.

ljmotta · 2024-12-18T15:46:26Z

@pefernan oh ok! So the job can be undefined or its properties can be undefined? If is the first case, please add the optional (?) to the job property, or if is the latter please change the job to Partial<Job>.

pefernan · 2024-12-18T16:00:58Z

@ljmotta nope priority, repeatLimit & repeatInterval can be undefined

ljmotta · 2024-12-18T18:29:00Z

@josedee Could you please update the Job interface with the correct type as @pefernan described? Them you can make the check as follow:

job.priority !== undefined && <></>

jomarko

I did no manual checks. However apache licenses and isNil removal seems as addressed.

pefernan · 2024-12-20T08:38:11Z

...oped-components/src/jobsManagement/envelope/components/JobsDetailsModal/JobsDetailsModal.tsx

-              </Split>
-            </FlexItem>
-            {job.repeatInterval && (
+            {job.priority !== undefined && (
              <FlexItem>


Priority still appears even if it's empty... @josedee is on PTO right now, will push a fix asap

@ljmotta ^

So maybe the priority can have the null value?

josedee added 2 commits December 18, 2024 11:36

Added nodeInstanceId to Job details, fixed bugs

f76a7aa

Merge remote-tracking branch 'origin/main' into kie-issues#1720

a772463

josedee requested a review from tiagobento as a code owner December 18, 2024 06:58

jomarko reviewed Dec 18, 2024

View reviewed changes

pefernan approved these changes Dec 18, 2024

View reviewed changes

Added License header back

096a625

ljmotta requested changes Dec 18, 2024

View reviewed changes

josedee added 2 commits December 19, 2024 12:45

Removed usage of isNil

197a0d6

Merge remote-tracking branch 'origin/main' into kie-issues#1720

2a2a680

jomarko approved these changes Dec 19, 2024

View reviewed changes

Fix tests

9939dcc

ljmotta approved these changes Dec 20, 2024

View reviewed changes

pefernan reviewed Dec 20, 2024

View reviewed changes

pefernan added 2 commits December 20, 2024 14:48

- Add null check

bf8aa0f

- Add missing dev ui Tasks user

ca6e181

pefernan merged commit c59b389 into apache:main Dec 23, 2024
15 checks passed

Abhitocode mentioned this pull request Dec 23, 2024

[kie-issues#1720] Enhance Process Details UI page to show nodeInstanc… Abhitocode/incubator-kie-tools#1

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[kie-issues#1720] Enhance Process Details UI page to show nodeInstanceId each timer belongs to #2820

[kie-issues#1720] Enhance Process Details UI page to show nodeInstanceId each timer belongs to #2820

josedee commented Dec 18, 2024

josedee commented Dec 18, 2024 •

edited

Loading

jomarko left a comment

jomarko Dec 18, 2024

pefernan Dec 18, 2024

josedee commented Dec 18, 2024 •

edited

Loading

pefernan left a comment

jomarko commented Dec 18, 2024

josedee commented Dec 18, 2024 •

edited

Loading

jomarko commented Dec 18, 2024

ljmotta left a comment

ljmotta Dec 18, 2024

ljmotta Dec 18, 2024

josedee Dec 18, 2024

ljmotta Dec 18, 2024

ljmotta Dec 18, 2024

pefernan commented Dec 18, 2024 •

edited

Loading

ljmotta commented Dec 18, 2024

pefernan commented Dec 18, 2024

ljmotta commented Dec 18, 2024

jomarko left a comment

pefernan Dec 20, 2024

ljmotta Dec 20, 2024

[kie-issues#1720] Enhance Process Details UI page to show nodeInstanceId each timer belongs to #2820

[kie-issues#1720] Enhance Process Details UI page to show nodeInstanceId each timer belongs to #2820

Conversation

josedee commented Dec 18, 2024

josedee commented Dec 18, 2024 • edited Loading

jomarko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josedee commented Dec 18, 2024 • edited Loading

pefernan left a comment

Choose a reason for hiding this comment

jomarko commented Dec 18, 2024

josedee commented Dec 18, 2024 • edited Loading

jomarko commented Dec 18, 2024

ljmotta 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

pefernan commented Dec 18, 2024 • edited Loading

ljmotta commented Dec 18, 2024

pefernan commented Dec 18, 2024

ljmotta commented Dec 18, 2024

jomarko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josedee commented Dec 18, 2024 •

edited

Loading

josedee commented Dec 18, 2024 •

edited

Loading

josedee commented Dec 18, 2024 •

edited

Loading

pefernan commented Dec 18, 2024 •

edited

Loading