Code review comment for lp:~ralsina/ubuntuone-windows-installer/local-folder-fixes

> * The comment added:
>
> # Block until we have server data828944
>
> may be misleading, since the app does not block but shows a loading message
> until the result from the backend is ready.

Changed.

> * If get_info fails, the overlay is never hid nor the user finds out that
> something went wrong. I filed bug #828944 so you can fix this in an incoming
> branch.

Thanks, will look into it.

> * test_exception_on_account_info should yield on get_info so we ensure that
> the call does not explodes "later". In this test, when solving bug #828944,
> you should check the overlay was hidden and a message was shown to the user
> (we may need input from design here).

Agreed.

> * there are no tests for the showing/hiding of the overlay inside change_page.

Added asserts to the changed_page tests.

> * there are no tests for add_folder when the params validate, calculate and
> volume_id are other than the defaults. Also, having volume_id defaulting to
> False may be a typo? volume_id, when defined, is a uuid...

I just wanted something to say "if not volume_id". I will default it to None.

> * it seems to me that update_sizes is not completely covered by tests. I think
> that the try-except for the RuntimeError is not tested, as well as the while
> True. For that last feature, we should have more than one thing being
> calculated its size, to show the sum of the sizes.

The RuntimeError only happens when a list item is deleted by Qt before its thread
returns the result (for example, if the user leaves the page and comes back),
and it just means we can ignore that item. I am not sure how to test that, because
if I delete the items manually, it will not cause that error, unless the thread
was running while I did it. It's pretty tricky to reproduce.

The while True breaks when the Queue is empty (which will happen eventually ;-),
or any other exception occurs. You mean add a test to ensure it ends?

> Also, we should confirm we're testing this case:
>
> "if the user has no UDFs created and selects nothing" -> we should show the
> quota used only

This was covered by test_total_size_unchecked (when there are UDFs, it should return the quota used)
and I now added test_total_size_unchecked_no_udfs (nothing checked, no UDFs, no quota used, should be 0)

> and that the show_hide_offer method received the proper parameter.

Added a fake show_hide_offer and asserts to check that.

> * correct me if I'm wrong, but seems like there is no test for
> on_folder_list_itemChanged

added test_on_folder_list_item_changed_col_0 and col_1

> nor on_add_folder_button_clicked.

This was already covered by test_add_folder_clicked_valid and test_add_folder_clicked_invalid.

> * question: would you agree to move all the tests for the local_folders module
> into its own test module, so we can follow the convention? The convention is:
> per every foo.py, there should be a test_foo.py.

Sure. Moved.