Fixes #30633 - Stopped chart doesnt count cancelled #578

MariaAga · 2020-08-11T13:50:13Z

in tasks dashboard the stopped chart only had counts for 'warning', 'error' and 'success', and was missing 'cancelled' and 'pending' result counts.
Added an 'other' counter for the total of 'cancelled' and 'pending'

Design was approved 😄

adamruzicka · 2020-08-13T11:05:20Z

Could we allow searching for "others" by clicking the number just like with other numbers in the table? Also, why not make it a part of the table?

MariaAga · 2020-08-13T11:08:29Z

Good idea, I'll add it as a filter. because we'll have to add 2 more rows ( 'cancelled' and 'pending') making the card too big, just adding the 'other' made the card bigger

adamruzicka · 2020-08-13T11:10:11Z

I meant doing it the same way as it is now, just adding it to the table instead of putting it below it. So you'd add just one row called "other"

MariaAga · 2020-08-13T11:18:05Z

I think that its a bit meaningless to count "Other total" and "other in the last 24 hours"

adamruzicka · 2020-08-13T11:32:44Z

I think that its a bit meaningless

Ok 👍

webpack/ForemanTasks/Components/TasksDashboard/TasksDashboardSelectors.js

app/services/foreman_tasks/dashboard_table_filter.rb

...nts/TasksDashboard/Components/TasksCardsGrid/Components/StoppedTasksCard/StoppedTasksCard.js

MariaAga · 2020-08-24T13:57:04Z

@LaViro can you take a look?

Ron-Lavi · 2020-08-24T17:59:51Z

Nice work @MariaAga,

I meant doing it the same way as it is now, just adding it to the table instead of putting it below it. So you'd add just one row called "other"

I agree with @adamruzicka,
it would fit in more logically to the table.
Also I think that the users don't really want to know about the all history of "other" stopped tasks,
IMO it would be more relevant to show them also the 24th column.

thinking about it - should we also add in the future a column for "last week" / "month" / "year" ?

should we rename it from "Other" to "unknown" ? since the task is ended but its result is neither "success" / "warning" / "error"
for me it "unknown" sounds a bit better.

MariaAga · 2020-08-25T11:44:33Z

Thanks @LaViro for the detailed feedback! this design and text is approved by Rox and Melanie.

Also I think that the users don't really want to know about the all history of "other" stopped tasks

This PR is for a user wanting to know how many cancelled tasks there are

IMO it would be more relevant to show them also the 24th column.

Because other includes different result types I don't think that a user will get any value from knowing how many 'other' result were there in the last 24h, but I do think the user should know that there are more result types than whats shown in the table.

thinking about it - should we also add in the future a column for "last week" / "month" / "year" ?

last week already exists in the table, and I think that the number of tasks in a year for a user will be too big for a dashboard

should we rename it from "Other" to "unknown" ? since the task is ended but its result is neither "success" / "warning" / "error"
for me it "unknown" sounds a bit better.

I don't think we should use unknown because we do know what the result is.

pondrejk

Checked from the user's pov, looks good

Ron-Lavi · 2020-08-25T13:55:13Z

...nts/TasksDashboard/Components/TasksCardsGrid/Components/StoppedTasksCard/StoppedTasksCard.js

+  const otherCount = data.other || 0;
+  delete data.other;


IMO it would be better to send the data correctly from the server, instead of manipulating it here.

What do you mean by getting it correctly? I can move the default 0 to the selector/ default props

the data object looks something like:
data = { error, success, warning, other }
instead of manipulating it here and later also using calcStoppedOther helper,
we could return from the server something like:
data = { results: { error, success, warning }, otherCount: 0 }

Ron-Lavi · 2020-08-25T13:55:59Z

...nts/TasksDashboard/Components/TasksCardsGrid/Components/StoppedTasksCard/StoppedTasksCard.js

+          <table className="table table-bordered table-striped stopped-table">
+            <thead>
+              <tr>
+                <th />
+                <th>{__('Total')}</th>
+                <th>{getQueryValueText(time)}</th>
+              </tr>
+            </thead>
+            <tbody>{StoppedTable(data, query, time, updateQuery)}</tbody>
+          </table>


This could go into its own component file.

Ron-Lavi · 2020-08-25T13:56:16Z

...nts/TasksDashboard/Components/TasksCardsGrid/Components/StoppedTasksCard/StoppedTasksCard.js

+          <span>
+            <OtherInfo />
+            <Button
+              bsStyle="link"
+              onClick={() =>
+                updateQuery({
+                  state: TASKS_DASHBOARD_AVAILABLE_QUERY_STATES.STOPPED,
+                  result: TASKS_DASHBOARD_AVAILABLE_QUERY_RESULTS.OTHER,
+                })
+              }
+            >
+              {otherCount}
+            </Button>
+          </span>


Also could go into its own component file.

Ron-Lavi · 2020-08-25T13:58:42Z

webpack/ForemanTasks/Components/TasksDashboard/TasksDashboardSelectors.js

+export const calcStoppedOther = data =>
+  (data?.cancelled?.total || 0) + (data?.pending?.total || 0);
+


Can we get this data from the server?

I think it will complicate the server more that this complicated the front end

commented about it also here: #578 (comment)

We don't need this anymore :)

Don't you pass it as otherCount from server?

Nope, the server returns {[result]:count} for every result in the DB

can you add a small test for calcStoppedOther?

MariaAga · 2020-08-26T13:25:34Z

After moving the components around this became a bit of a refactor PR as well

...nts/TasksDashboard/Components/TasksCardsGrid/Components/StoppedTasksCard/StoppedTasksCard.js

Ron-Lavi · 2020-08-27T07:25:19Z

webpack/ForemanTasks/Components/TasksDashboard/TasksDashboardSelectors.js

+export const calcStoppedOther = data =>
+  (data?.cancelled?.total || 0) + (data?.pending?.total || 0);
+


We don't need this anymore :)

Ron-Lavi

Almost there !

Left some inline comments,
also, looks like the font is not on the same size?

while at it, can you add a bit margin near the rest of the results icons?
to have the same space between the icon and the text as in others:

...Components/TasksDashboard/Components/TasksCardsGrid/Components/StoppedTasksCard/OtherInfo.js

Ron-Lavi · 2020-09-01T15:27:04Z

webpack/ForemanTasks/Components/TasksDashboard/TasksDashboardSelectors.js

+export const calcStoppedOther = data =>
+  (data?.cancelled?.total || 0) + (data?.pending?.total || 0);
+


can you add a small test for calcStoppedOther?

MariaAga · 2020-09-02T13:40:20Z

Font is 12px in both.
Added margin to the icons
Added test for calcStoppedOther

...Components/TasksDashboard/Components/TasksCardsGrid/Components/StoppedTasksCard/OtherInfo.js

webpack/ForemanTasks/Components/TasksDashboard/__tests__/TasksDashboardSelectors.test.js

MariaAga · 2020-09-03T10:17:59Z

Same font for everyone 🎉

Ron-Lavi

Thanks for all of the work @MariaAga !
Works great :)

adamruzicka

Works as expected, ACK

adamruzicka · 2020-09-15T08:53:56Z

Thank you @MariaAga & @LaViro !

theforeman-bot added Needs testing Not yet reviewed labels Aug 11, 2020

theforeman-bot added the Waiting on contributor label Aug 17, 2020

MariaAga force-pushed the 30633 branch from 339980e to c145b0f Compare August 17, 2020 09:56

theforeman-bot added Waiting on contributor and removed Waiting on contributor labels Aug 17, 2020

MariaAga force-pushed the 30633 branch from c145b0f to feec426 Compare August 17, 2020 10:04

theforeman-bot added Waiting on contributor and removed Waiting on contributor labels Aug 17, 2020

adamruzicka reviewed Aug 19, 2020

View reviewed changes

MariaAga force-pushed the 30633 branch from feec426 to 7f73ab6 Compare August 19, 2020 11:36

theforeman-bot added Waiting on contributor and removed Waiting on contributor labels Aug 19, 2020

pondrejk approved these changes Aug 25, 2020

View reviewed changes

theforeman-bot removed the Not yet reviewed label Aug 25, 2020

Ron-Lavi reviewed Aug 25, 2020

View reviewed changes

theforeman-bot added Needs re-review Waiting on contributor and removed Waiting on contributor labels Aug 26, 2020

Ron-Lavi reviewed Aug 27, 2020

View reviewed changes

MariaAga force-pushed the 30633 branch from 768fb59 to 019c58a Compare September 1, 2020 12:25

theforeman-bot removed the Waiting on contributor label Sep 1, 2020

Ron-Lavi reviewed Sep 1, 2020

View reviewed changes

MariaAga force-pushed the 30633 branch from 019c58a to 54ffef3 Compare September 2, 2020 13:39

Ron-Lavi reviewed Sep 2, 2020

View reviewed changes

...Components/TasksDashboard/Components/TasksCardsGrid/Components/StoppedTasksCard/OtherInfo.js Show resolved Hide resolved

webpack/ForemanTasks/Components/TasksDashboard/__tests__/TasksDashboardSelectors.test.js Outdated Show resolved Hide resolved

Fixes #30633 - Stopped chart doesnt count cancelled

22ee978

MariaAga force-pushed the 30633 branch from 54ffef3 to 22ee978 Compare September 3, 2020 10:17

Ron-Lavi approved these changes Sep 13, 2020

View reviewed changes

theforeman-bot removed the Needs re-review label Sep 13, 2020

adamruzicka approved these changes Sep 15, 2020

View reviewed changes

adamruzicka merged commit 05dbdb2 into theforeman:master Sep 15, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #30633 - Stopped chart doesnt count cancelled #578

Fixes #30633 - Stopped chart doesnt count cancelled #578

MariaAga commented Aug 11, 2020 •

edited

adamruzicka commented Aug 13, 2020

MariaAga commented Aug 13, 2020

adamruzicka commented Aug 13, 2020

MariaAga commented Aug 13, 2020

adamruzicka commented Aug 13, 2020

MariaAga commented Aug 24, 2020

Ron-Lavi commented Aug 24, 2020

MariaAga commented Aug 25, 2020

pondrejk left a comment

Ron-Lavi Aug 25, 2020

MariaAga Aug 26, 2020

Ron-Lavi Aug 26, 2020

Ron-Lavi Aug 25, 2020

Ron-Lavi Aug 25, 2020

Ron-Lavi Aug 25, 2020

MariaAga Aug 26, 2020

Ron-Lavi Aug 26, 2020

Ron-Lavi Aug 27, 2020

MariaAga Aug 27, 2020

Ron-Lavi Aug 27, 2020

MariaAga Aug 27, 2020

Ron-Lavi Sep 1, 2020

MariaAga commented Aug 26, 2020

Ron-Lavi Aug 27, 2020

Ron-Lavi left a comment

Ron-Lavi Sep 1, 2020

MariaAga commented Sep 2, 2020 •

edited

MariaAga commented Sep 3, 2020

Ron-Lavi left a comment

adamruzicka left a comment

adamruzicka commented Sep 15, 2020

		export const calcStoppedOther = data =>
		(data?.cancelled?.total \|\| 0) + (data?.pending?.total \|\| 0);

Fixes #30633 - Stopped chart doesnt count cancelled #578

Fixes #30633 - Stopped chart doesnt count cancelled #578

Conversation

MariaAga commented Aug 11, 2020 • edited

adamruzicka commented Aug 13, 2020

MariaAga commented Aug 13, 2020

adamruzicka commented Aug 13, 2020

MariaAga commented Aug 13, 2020

adamruzicka commented Aug 13, 2020

MariaAga commented Aug 24, 2020

Ron-Lavi commented Aug 24, 2020

MariaAga commented Aug 25, 2020

pondrejk 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

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

MariaAga commented Aug 26, 2020

Choose a reason for hiding this comment

Ron-Lavi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MariaAga commented Sep 2, 2020 • edited

MariaAga commented Sep 3, 2020

Ron-Lavi left a comment

Choose a reason for hiding this comment

adamruzicka left a comment

Choose a reason for hiding this comment

adamruzicka commented Sep 15, 2020

MariaAga commented Aug 11, 2020 •

edited

MariaAga commented Sep 2, 2020 •

edited