People management network and data layer

[oguzkocer]

Relevant #3901. This PR introduces the network & data layer for the People Management. Please keep in mind that this is the first iteration of both components and it's not being merged to develop. Basically, with this PR, we'll be making the request to /sites/$site/users, convert the response to Person objects and save them in our DB.

There was an interesting choice between rather using a remote site ID and using the local blog id. At first I've thought that site ID made the most sense, but thinking about self-hosted sites I wasn't sure if the combination of the user's remote id and the remote site ID would be unique. So, to make sure it'll work well with self-hosted sites, I decided to change that to the local blog id instead.

To test: Pick a blog and go into "People" page from there. You should see the users for that site and selecting a user should bring up the detail page.

Note that I haven't touched the UI layer yet, it's as it was before. I intend to change that once this is merged in.

Needs review: @hypest

/cc @astralbodies

[hypest]

I think that this override makes the code more complex and "dangerous" than is needed, since it breaks the [notifyDataSetChanged]'s documentation (http://developer.android.com/intl/es/reference/android/widget/BaseAdapter.html#notifyDataSetChanged()) by mutating the Adapter instead of just notifying the observers. The caller should assume that the adapter's data have already been changed by the time notifyDataChanged() is called but this override invalidates that assumption.

I think the data should be provided to the Adapter (via a constructor or setter) and if needed as a convenience, reloaded by a helper method (say, "reloadFromDB(localBlogId)" for example) whose explicit function would be to load the list from the DB. The local mLocalBlogId variable would not even be needed then.
The (unoverridden) notifyDataSetChanged could then be called by that helper or be left to the helper's caller to do it.

[hypest]

@oguzkocer , thanks for this PR. I didn't feel the need to jump out and look for hidden information elsewhere to understand the code and that helped me a lot with the review!

I've finished my pass. I left comments with varied "importance", some minor and some not so minor. Some you've already addressed as I continued reviewing. Thanks!

Last comment I would add here, and this is not a merge-blocker per se, is that I would like to see some very basic unit tests added if you have the chance. For example, I'd like to see a test that would stress the "get data from network, save to DB and load to see if valid" path. Ping me for extra info if needed. Cheers!

[oguzkocer]

@hypest Thanks for the detailed PR review. I've fixed most of the issues you raised and left some comments/questions on the other ones. Once we agree on those I'll have a look at unit testing. I've not done any yet in Android, so hopefully I can get that done rather quickly. Cheers!

[hypest]

This is not a remark per se but, perhaps it could be useful to add the @Nullable annotation to the method to "nudge" the caller to check for NPE. Not a biggie nor a blocker and totally up to you to add it or not. Cheers!

[hypest]

Thank you for the changes @oguzkocer ! I've finished my pass over them and left some comments. I can review again if any new changes arise out of those comments.

...I'll have a look at unit testing.

Woot! Feel free to ping me about it! Cheers!

[hypest]

Finished my pass @oguzkocer , LGTM!

[oguzkocer]

I've looked into adding some tests but it's not a practice we're doing often and I especially don't like testing the network requests. I think if we are going to start adding unit tests to our PRs it should be a team effort and we should document that somewhere so we follow the same practices.

For now, I am going to leave this PR as is, but before we merge the whole feature into develop I might re-visit it. /cc @hypest

[hypest]

LGTM