Opened 4 years ago

Closed 4 years ago

#12955 closed Bug (fixed)

[Notification Aggregator] Total weight is not updated when task is canceled

Reported by: Piotr Jasiun Owned by: Piotrek Koszuliński
Priority: Normal Milestone: CKEditor 4.5.0 Beta
Component: General Version: 4.5.0 Beta
Keywords: Cc:

Description (last modified by Piotr Jasiun)

Total weight is not updated when task is canceled. I pushed a tests which show the problem.

Change History (15)

comment:1 Changed 4 years ago by Piotr Jasiun

Description: modified (diff)
Summary: [Notification Aggregator] total weight is not updated when task is canceled[Notification Aggregator] Total weight is not updated when task is canceled

comment:2 Changed 4 years ago by Jakub Ś

Status: newconfirmed

comment:3 Changed 4 years ago by Piotr Jasiun

Description: modified (diff)

I think that using redundant data is a bad smell. Is there any reason to store _totalWeights in the separate variable, not counting it every time based on _tasks? Such variables may be introduce to fix some performance issue, but it should be added when the fragment of code met performance problem, not before, "just in case". Otherwise we can have bugs like this.

Last edited 4 years ago by Piotr Jasiun (previous) (diff)

comment:4 Changed 4 years ago by Piotrek Koszuliński

I think that using redundant data is a bad smell. Is there any reason to store _totalWeights in the separate variable, not counting it every time based on _tasks? Such variables may be introduce to fix some performance issue, but it should be added when the fragment of code met performance problem, not before, "just in case". Otherwise we can have bugs like this.

I proposed introducing it because counting all the time the sums (there are 3 sums iirc) wasn't reasonable. And I don't mean performance. I mean sanity.

Anyway, one broken case doesn't mean that the whole approach is bad, so please just fix it ;).

comment:5 Changed 4 years ago by Artur Delura

Owner: set to Artur Delura
Status: confirmedassigned

comment:6 Changed 4 years ago by Artur Delura

Status: assignedreview

Changes in branch:t/12955.

comment:7 Changed 4 years ago by Piotr Jasiun

Status: reviewreview_failed

We have 3 manual tests for notification aggregator, but none of then check how aggregator bahave if we remove task: http://tests.ckeditor.dev:1030/#tests/is:manual,path:/tests/plugins/notificationaggregator

There should be manual tests for removing tasks.

comment:8 Changed 4 years ago by Piotr Jasiun

Related ticket: #13009.

comment:9 Changed 4 years ago by Artur Delura

Status: review_failedassigned

comment:10 Changed 4 years ago by Artur Delura

Done. Changes in the same branch.

comment:11 Changed 4 years ago by Artur Delura

Status: assignedreview

comment:12 Changed 4 years ago by Piotrek Koszuliński

Status: reviewreview_failed

comment:13 Changed 4 years ago by Piotrek Koszuliński

Besides that, the manual test's description could be simplified and the step timeout lengthen because it's hard to follow what's happening. In other words - it must be clear what's the expected result and when to expect it.

Last edited 4 years ago by Piotrek Koszuliński (previous) (diff)

comment:14 Changed 4 years ago by Piotrek Koszuliński

Owner: changed from Artur Delura to Piotrek Koszuliński
Status: review_failedassigned

comment:15 Changed 4 years ago by Piotrek Koszuliński

Resolution: fixed
Status: assignedclosed
Version: 4.5.0 (GitHub - major)

Fixed on major with git:8559966 after making several improvements in the tests. KISS!

Note: See TracTickets for help on using tickets.
© 2003 – 2019 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy