The implementation of `ThreadUI.delete` and `ThreadUI.resendMessage` both contain logic for removing messages. This logic should be split out into dedicated methods that can be shared and tested.

blocks leo+ Bug 868218 -> leo+

Comment on attachment 751297 [details] [diff] [review] ThreadUI.removeMessage Review of attachment 751297 [details] [diff] [review]: ----------------------------------------------------------------- the tests are failing : ✖ 2 of 230 tests failed: in sms_test.js : 1) [sms] SMS App Unit-Test Messages given a thread Thread-messages Edit mode (bubbles view) Select all while receiving new message: Error: TypeError: this is undefined (http://sms.gaiamobile.org:8080/js/thread_ui.js?time=1368870784883:952) at onerror (http://test-agent.gaiamobile.org:8080/common/vendor/mocha/mocha.js:4959) in thread_ui_test.js : 2) [sms] "before each" hook: Error: Error: expected 0 to equal 1 (http://test-agent.gaiamobile.org:8080/common/test/helper.js?time=1368870784429:30) at onerror (http://test-agent.gaiamobile.org:8080/common/vendor/mocha/mocha.js:4959) Probably because of the `deleteMessages` function not being bound. ::: apps/sms/js/thread_ui.js @@ +947,5 @@ > MessageManager.getThreads(ThreadListUI.renderThreads, > function afterRender() { > var completeDeletionDone = false; > // Then sending/received messages > for (var i = 0; i < inputs.length; i++) { while you're there, do you feel like caching the input length ? we also have the same loop before in the outer function, could make sense to cache the length only once. Remember we might have thousands of messages :) @@ +948,5 @@ > function afterRender() { > var completeDeletionDone = false; > // Then sending/received messages > for (var i = 0; i < inputs.length; i++) { > + this.removeMessage(inputs[i].parentNode.parentNode); this function is not bound to `this`, you nees to use either ThreadUI or bind this method. (have you tested this ? :) because it probably doesn't work ;) ) @@ +953,2 @@ > } > + if (!ThreadUI.container.childNodes.length) { use `if (!ThreadUI.container.firstElementChild)` (or with `this` if you bind the function) @@ +1190,2 @@ > > + // Is the last one in the ul? I wonder if we could not invert the way it works : * first remove the message * then check messagesContainer.firstElementChild * if it's not there, remove the container and the header My try here is of course to not use `childNodes.length` :) another idea is to test for `messagesContainer.firstElementChild.nextElementSibling`. @@ +1195,5 @@ > + this.container.removeChild(header); > + this.container.removeChild(messagesContainer); > + } else { > + // If not we only have to remove the message > + messageDOM.parentNode.removeChild(messageDOM); you can reuse messagesContainer here anyway. @@ +1201,3 @@ > > + // Have we more elements in the view? > + if (!this.container.childNodes.length) { hey, see, you could reuse this here :) (with `firstElementChild` of course) @@ +1202,5 @@ > + // Have we more elements in the view? > + if (!this.container.childNodes.length) { > + // Update header index > + this.dayHeaderIndex = 0; > + this.timeHeaderIndex = 0; I wondered what these properties were for, and a grep showed me that this is not used at all. Could you double-check and clean this up ? @@ +1209,3 @@ > > + resendMessage: function thui_resendMessage(id) { > + if (typeof id !== 'number') { to do this, Rick uses to do `+id`, I feel this is more readable, but this is up to you here. ::: apps/sms/test/unit/thread_ui_test.js @@ +720,4 @@ > > + suite('removeMessage', function() { > + setup(function() { > + ThreadUI.container.innerHTML = '<h2></h2><ul><li></li><li></li></ul>'; it's probably not necessary, but just to be clean, could you add a teardown to clear up the container ? Also, if you use the container a lot (and you are) you can create a suite-scoped `container` var like `sendButton`, `recipient` etc.

Comment on attachment 751297 [details] [diff] [review] ThreadUI.removeMessage Review of attachment 751297 [details] [diff] [review]: ----------------------------------------------------------------- ::: apps/sms/js/thread_ui.js @@ +947,5 @@ > MessageManager.getThreads(ThreadListUI.renderThreads, > function afterRender() { > var completeDeletionDone = false; > // Then sending/received messages > for (var i = 0; i < inputs.length; i++) { new patch @@ +948,5 @@ > function afterRender() { > var completeDeletionDone = false; > // Then sending/received messages > for (var i = 0; i < inputs.length; i++) { > + this.removeMessage(inputs[i].parentNode.parentNode); Oops, this was the cause of the problems in the units - I >had< tested it, then I changed this right before commit. @@ +953,2 @@ > } > + if (!ThreadUI.container.childNodes.length) { done in new commit @@ +1190,2 @@ > > + // Is the last one in the ul? Yeah, thats a lot simpler :) -- I had just copied the code that already existed... But might as well clean it up too... @@ +1195,5 @@ > + this.container.removeChild(header); > + this.container.removeChild(messagesContainer); > + } else { > + // If not we only have to remove the message > + messageDOM.parentNode.removeChild(messageDOM); new commit @@ +1201,3 @@ > > + // Have we more elements in the view? > + if (!this.container.childNodes.length) { removed anyway.... @@ +1202,5 @@ > + // Have we more elements in the view? > + if (!this.container.childNodes.length) { > + // Update header index > + this.dayHeaderIndex = 0; > + this.timeHeaderIndex = 0; I also found no references other than setting it to 0 @@ +1209,3 @@ > > + resendMessage: function thui_resendMessage(id) { > + if (typeof id !== 'number') { I actually didn't change this, no idea why this was in the diff, but I made it +id anyway

I'm trying to weigh the benefits and drawbacks of building `MessageManager.delete` into this method. On the one hand, I don't see any scenarios where you would want to remove the Message element but *not* its entry in the database. Because deleting messages is asynchronous, combining functionality this way would benefit readability and reduce the surface for bugs in the consuming code. At the same time, you might say this further couples MessageManager and ThreadUI. But the more I think about it, the two are *already* coupled--it's just that the consuming code has to do the coupling. As I said before, I think you always want to perform the two operations at the same time. If this is the case, the coupling is unavoidable, and possibly indicative of a larger-scale re-factoring. If you agree, it would probably also make sense (from an API perspective) to define `ThreadUI.removeMessage` to accept a message ID (instead of a message element). What do you think about this, Corey?

Comment on attachment 751352 [details] [diff] [review] patch Review of attachment 751352 [details] [diff] [review]: ----------------------------------------------------------------- Stealing the review. Please address the comments and I will take a look again! Thanks. ::: apps/sms/js/thread_ui.js @@ +951,3 @@ > } > + if (!ThreadUI.container.firstElementChild) { > + var mainWrapper = document.getElementById('main-wrapper'); We could cache this var due to we are using it in several parts of the code. @@ +951,4 @@ > } > + if (!ThreadUI.container.firstElementChild) { > + var mainWrapper = document.getElementById('main-wrapper'); > + mainWrapper.classList.remove('edit'); Do we need this line? Because when changing the hash to '#thread-list' we will go through https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/js/message_manager.js#L236 right? Sp probably we could remove this line and the previous 'main-wrapper' declaration. @@ +956,1 @@ > window.history.back(); ?? Previously you have a window.location.hash and here there is a window. history.back... This part is *only* needed to be executed in the case of *not removing* all messages in a thread. Imagine that you have 3 messages in a thread, and you remove only 1. In this case you will execute: window.history.back(); However, if you have 3 messages in a thread, and you remove ALL of them, you will execute: window.location.hash = '#thread-list'; In both calls, you need to hide the 'WaitingScreen', so you need to add WaitingScreen.hide(); Probably you coud do something like: WaitingScreen.hide(); if (!ThreadUI.container.firstElementChild) { window.location.hash = '#thread-list'; return; } window.history.back();

Please rebase your patch because I have some errors trying to apply this patch on master. For having a quick review, Could you create a PR with the comments applied? Thanks!

Comment on attachment 751352 [details] [diff] [review] patch Review done. Let me know when the changes will be ready and I will review it again! Thanks.

(In reply to Mike Pennisi [:jugglinmike] from comment #6) > I'm trying to weigh the benefits and drawbacks of building > `MessageManager.delete` into this method. > > On the one hand, I don't see any scenarios where you would want to remove > the Message element but *not* its entry in the database. Because deleting > messages is asynchronous, combining functionality this way would benefit > readability and reduce the surface for bugs in the consuming code. So, for the not-downloaded messages, you don't want to delete the message ID - it stays the same, it just comes in as a status change. > If you agree, it would probably also make sense (from an API perspective) to > define `ThreadUI.removeMessage` to accept a message ID (instead of a message > element). Maybe I should just add DOM to the function name, I wanted to abstract the removeMessageDOM portion, we could then use this as a callback for a wrapper around deleteMessage at a later date if you wanted to further improve this later.

(In reply to Borja Salguero [:borjasalguero] from comment #8) > Please rebase your patch because I have some errors trying to apply this > patch on master. For having a quick review, Could you create a PR with the > comments applied? Thanks! I rebase every time I submit a patch - if it went stale, it did so recently - When you applied the patch, did you use git am -3 ?

(In reply to Mike Pennisi [:jugglinmike] from comment #6) > I'm trying to weigh the benefits and drawbacks of building > `MessageManager.delete` into this method. > > On the one hand, I don't see any scenarios where you would want to remove > the Message element but *not* its entry in the database. Because deleting > messages is asynchronous, combining functionality this way would benefit > readability and reduce the surface for bugs in the consuming code. > > At the same time, you might say this further couples MessageManager and > ThreadUI. But the more I think about it, the two are *already* coupled--it's > just that the consuming code has to do the coupling. As I said before, I > think you always want to perform the two operations at the same time. If > this is the case, the coupling is unavoidable, and possibly indicative of a > larger-scale re-factoring. > > If you agree, it would probably also make sense (from an API perspective) to > define `ThreadUI.removeMessage` to accept a message ID (instead of a message > element). > > What do you think about this, Corey? I think that one plan should be to remove MessageManager.. ie the gecko API has all we need to be able to call it easily. However, I'm aware of a long term plan to remove the specific SMS/MMS API and to replace it with a more generic DataSource API, then it makes sense to keep the API abstracted in a proper object.

(In reply to Borja Salguero [:borjasalguero] from comment #7) > Comment on attachment 751352 [details] [diff] [review] > patch > > Review of attachment 751352 [details] [diff] [review]: > ----------------------------------------------------------------- > > Stealing the review. Please address the comments and I will take a look > again! Thanks. > > ::: apps/sms/js/thread_ui.js > @@ +951,3 @@ > > } > > + if (!ThreadUI.container.firstElementChild) { > > + var mainWrapper = document.getElementById('main-wrapper'); > > We could cache this var due to we are using it in several parts of the code. > > @@ +951,4 @@ > > } > > + if (!ThreadUI.container.firstElementChild) { > > + var mainWrapper = document.getElementById('main-wrapper'); > > + mainWrapper.classList.remove('edit'); > > Do we need this line? Because when changing the hash to '#thread-list' we > will go through > https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/js/message_manager. > js#L236 right? Sp probably we could remove this line and the previous > 'main-wrapper' declaration. > > @@ +956,1 @@ > > window.history.back(); > > ?? Previously you have a window.location.hash and here there is a window. > history.back... This part is *only* needed to be executed in the case of > *not removing* all messages in a thread. > Imagine that you have 3 messages in a thread, and you remove only 1. In this > case you will execute: > window.history.back(); > > However, if you have 3 messages in a thread, and you remove ALL of them, you > will execute: > window.location.hash = '#thread-list'; > > In both calls, you need to hide the 'WaitingScreen', so you need to add > WaitingScreen.hide(); > > Probably you coud do something like: > WaitingScreen.hide(); > if (!ThreadUI.container.firstElementChild) { > window.location.hash = '#thread-list'; > return; > } > window.history.back(); All of your comments exist in code I was just moving around, I err'ed on the side of caution not changing it's behavior. Considering you've probably had some hand in writing this chunk, I'll gladly take your advice to simplify it further.

Adresses issues from borja

looks good to me, I'm trying it on the device now.

Comment on attachment 751662 [details] [diff] [review] patch Review of attachment 751662 [details] [diff] [review]: ----------------------------------------------------------------- Small comments. Please address and I will take a look again. ::: apps/sms/js/thread_ui.js @@ +954,2 @@ > } > + WaitingScreen.hide(); I would recommend to put this line at the end, for making the hash change and then removing this window. The transition is smooth. @@ +958,1 @@ > } If the firstElementChild exist, you have to call window.history.back().

I've tested your patch, and for me it's failing. STR: - Open SMS App - Send a SMS to 'a' with text 'Hola' - Remove 'Hola' form 'a' thread EXPECTED: Due to the thread is empty, I will go back to 'thread-list' properly updated. CURRENTLY: Is stucked in thread 'a'. Please take a look. Thanks!

* when deleting all messages in one thread, we should go back to the thread list view. It's been broken now.

* turn on flight mode * send a sms to a new number (that you don't have in the thread list yet) => the header text is not updated whereas it works with the master code

Think I got the bugs ironed out in here.

The patch looks good, I'll test it on my phone in about 30 minutes, do you feel like adding unit tests for these 2 bugs before that ?

Sorry Corey, but my STR in Comment 19 still doesn't work in your patch whereas it works in master :( Not sure what causes it. I'll test it again later if you upload a new patch. Making a unit tests might help.

(In reply to Julien Wajsberg [:julienw] from comment #22) > Sorry Corey, but my STR in Comment 19 still doesn't work in your patch > whereas it works in master :( > > Not sure what causes it. I'll test it again later if you upload a new patch. > Making a unit tests might help. I'm really not sure whats happening here, trying to debug

(In reply to Julien Wajsberg [:julienw] from comment #22) > Sorry Corey, but my STR in Comment 19 still doesn't work in your patch > whereas it works in master :( > > Not sure what causes it. I'll test it again later if you upload a new patch. > Making a unit tests might help. I've been able to reproduce this on master, and have tracked it down to an onerror getting called on the cursor in the MessageManager.getThreads() - it's intermittent and happens only about 50% of the time.

I just reproduced this bug with the master code after all. Therefore I'll file a new bug for this and I'll r+ your patch.

Comment on attachment 751698 [details] [diff] [review] patch r=me, go for it !

(In reply to Julien Wajsberg [:julienw] from comment #19) > * turn on flight mode > * send a sms to a new number (that you don't have in the thread list yet) > > => the header text is not updated whereas it works with the master code bug 874186

master: e4a0c0496f403a1b0b1326ec773f5e93c9afd9c6

Filed Bug 874187 for the onerror bug.

Thanks Corey, I duped my bug as yours was filed earlier :)

v1-train: cf2de66