Opened 8 years ago
Closed 8 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 )
Total weight is not updated when task is canceled. I pushed a tests which show the problem.
Change History (15)
comment:1 Changed 8 years ago by
| 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 8 years ago by
| Status: | new → confirmed |
|---|
comment:3 Changed 8 years ago by
| Description: | modified (diff) |
|---|
comment:4 Changed 8 years ago by
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 8 years ago by
| Owner: | set to Artur Delura |
|---|---|
| Status: | confirmed → assigned |
comment:7 Changed 8 years ago by
| Status: | review → review_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:9 Changed 8 years ago by
| Status: | review_failed → assigned |
|---|
comment:11 Changed 8 years ago by
| Status: | assigned → review |
|---|
comment:12 Changed 8 years ago by
| Status: | review → review_failed |
|---|
I rebased the branch and:
- http://tests.ckeditor.dev:1030/tests/plugins/notificationaggregator/aggregator#tests%2Fplugins%2Fnotificationaggregator%2Faggregator%20-%20test%20cancel%20done%20task%20substract%20doneTasks fails,
- there's no test what happens if the removed task was begun (done in >0%).
comment:13 Changed 8 years ago by
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.
comment:14 Changed 8 years ago by
| Owner: | changed from Artur Delura to Piotrek Koszuliński |
|---|---|
| Status: | review_failed → assigned |
comment:15 Changed 8 years ago by
| Resolution: | → fixed |
|---|---|
| Status: | assigned → closed |
| Version: | → 4.5.0 (GitHub - major) |
Fixed on major with git:8559966 after making several improvements in the tests. KISS!
I think that using redundant data is a bad smell. Is there any reason to store
_totalWeightsin 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.