3576 Fix bulk order management race condition #3590
Conversation
The "Loading orders" spinner is still visible from the beginning so that it can be used as indicator for when the page is loaded. Before, the "No orders found" message was visible between page load and Angular initilisation.
DRYing the code allows for easier refactor and more consistency.
This seems to make the spec 4 times more reliable. Still failing from time to time.
|
@luisramos0 I think you are right about testing. I also attended to the style issues. Ready for review again. |
|
oh, we should update the "What should we test?" section. |
This also minimises the risk of values changing in the meantime.
|
some issues with the karma specs, they probably need to be adapted. |
The bulk order management page tried to wait for orders being loaded. But instead of receiving a request object with an a promise to wait for the page received a simple array without promise. As a result, the page didn't wait for orders being loaded. When loading of orders took longer than loading of line items they or at least not all of them were enriched with distributors and order cycles and the line items were missing some orders. That lead to random spec failures.
|
So great to have specs! I didn't know that orders were returned within an object instead of as an array. I amended my last commit to just provide the needed promise. I hope it's all good now. 🤞 Ready for review again. |
|
No Bulk Order failures in 10 builds 🎉 |
mkllnk
changed the title
|
Tests are passing so this should be merged right away |
|
we can merge and test afterwards if you want. but i vote for a testing round. It's a tricky area this page loading business... |
There was a problem hiding this comment.
Nice investigation, @mkllnk! 🎉
I left a comment re: a line I'm not sure should be removed - let me know what you think. 😀 But don't worry, as far as I see, putting that line back if you agree would not affect the green builds.
| @@ -9,7 +9,7 @@ angular.module("admin.resources").factory 'Orders', ($q, OrderResource, RequestM | |||
| request = OrderResource.index params, (data) => | |||
| @load(data) | |||
| (callback || angular.noop)(data) | |||
| RequestMonitor.load(request.$promise) | |||
There was a problem hiding this comment.
Is it really okay to remove this line? It doesn't seem necessary to remove, and many pages including the bulk order management use RequestMonitor.
There was a problem hiding this comment.
That call was redundant. The only call of this method is in the line items controller and it uses the RequestMonitor already:
|
I agree with @luisramos0 about testing. 🙂 |
|
Yes, pretty much all bulk order management specs where affected by this bug. |
What? Why?
Fixes #3576.
We have a very flaky spec that causes half of our build to fail. I think I fixed it now. The main problem was a method returning an array instead of a query object with a promise. Without the promise the page didn't wait for the request to complete.
What should we test?
Bulk order management page should show the "loading" message correctly and only show "no records found" if no records are listed. There is a change in the date filters as well.
Release notes
Fixed: Improved reliability of our automated tests.
Changelog Category: Fixed