Adds request queueing to API resource fetch, adds save method to Models. #279
Conversation
| }, (response) => { | ||
| logging.error('An error occurred', response); | ||
| reject(response); | ||
| const promise = new Promise((resolve, reject) => { |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
Current coverage is 82.14% (diff: 13.40%)@@ master #279 diff @@
==========================================
Files 92 96 +4
Lines 2950 3209 +259
Methods 151 181 +30
Messages 0 0
Branches 345 367 +22
==========================================
+ Hits 2450 2636 +186
- Misses 477 550 +73
Partials 23 23
|
| // Clean up the reference to this promise | ||
| this.promises.splice(this.promises.indexOf(promise), 1); | ||
| }, (response) => { | ||
| logging.error('An error occurred', response); |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
indirectlylit
changed the title
| // Clean up the reference to this promise | ||
| this.promises.splice(this.promises.indexOf(promise), 1); | ||
| }); | ||
| } | ||
| }); |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
|
Since this is targeted post-MMVP, I don't feel so bad asking: any chance we can look at how we might write client-side tests for code like this? We have really good coverage on the server-side stuff, and very minimal client-side coverage. Lots of complicated/possibly fragile logic in here, and would be good to have it covered. |
|
I left the checkbox for tests in place for precisely that reason, yes. There is currently 0 code coverage for the entire |
|
Nice -- will be good to have some solid examples of client-side mocking/testing to inspire others. Perhaps putting some notes into the docs about how to approach this, as a reference, in the process. |
|
You don't like the ones I already did? https://github.com/learningequality/kolibri/blob/master/kolibri/core/assets/test/core_app_mediator.js :( |
|
Although, I admit the docs could be expanded upon a little: https://github.com/learningequality/kolibri/blob/master/docs/dev/frontend.rst#unit-testing |
|
Cool, hadn't seen the tests or docs -- looks great. Yeah, some unpacking/elaboration of the docs instructions may be good over time. And, when you're back in August, perhaps a hack session tech talk. |
indirectlylit
changed the title
| @@ -28,35 +28,106 @@ class Model { | |||
|
|
|||
| // force IDs to always be strings - this should be changed on the server-side too | |||
| this.attributes[this.resource.idKey] = String(this.attributes[this.resource.idKey]); | |||
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
Summary
This adds the request queueing that @jamalex had previously suggested, to handle the situation where multiple code points call fetch on the same object.
In addition, with this in hand, I implemented a save method for the Model class, which accepts an object with the properties to save, it also obeys the request queue, so any existing fetches or saves will be resolved before it is initiated.
TODO