Opened 10 years ago
Closed 10 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 10 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 10 years ago by
| Status: | new → confirmed |
|---|
comment:3 Changed 10 years ago by
| Description: | modified (diff) |
|---|
comment:4 Changed 10 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 10 years ago by
| Owner: | set to Artur Delura |
|---|---|
| Status: | confirmed → assigned |
comment:7 Changed 10 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 10 years ago by
| Status: | review_failed → assigned |
|---|
comment:11 Changed 10 years ago by
| Status: | assigned → review |
|---|
comment:12 Changed 10 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 10 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 10 years ago by
| Owner: | changed from Artur Delura to Piotrek Koszuliński |
|---|---|
| Status: | review_failed → assigned |
comment:15 Changed 10 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.