fix: Update identifiers list on adding or removing items #743

tarun0 · 2017-08-07T09:14:47Z

Apply the MifosStyle.xml style template to your code in Android Studio.
Run the unit tests with ./gradlew check to make sure you didn't break anything
If you have multiple commits please combine them into one commit by squashing them.

tarun0 · 2017-08-07T10:26:35Z

droidchef · 2017-08-07T11:24:23Z

...com/mifos/mifosxdroid/dialogfragments/identifierdialog/ClientIdentifierCreationListener.java

+
+    void onClientIdentifierCreationSuccess();
+
+    void onClientIdentifierCreationFailure();


Amazing 😀 That you care about failures and not just success.

droidchef · 2017-08-07T11:24:43Z

...in/java/com/mifos/mifosxdroid/dialogfragments/identifierdialog/IdentifierDialogFragment.java

@@ -52,6 +52,8 @@
    @Inject
    IdentifierDialogPresenter mIdentifierDialogPresenter;

+    ClientIdentifierCreationListener clientIdentifierCreationListener;


Can this be private and also @Nullable since I see you are doing a null check below.

droidchef · 2017-08-07T11:27:50Z

...in/java/com/mifos/mifosxdroid/dialogfragments/identifierdialog/IdentifierDialogFragment.java

@@ -52,6 +52,8 @@
    @Inject
    IdentifierDialogPresenter mIdentifierDialogPresenter;

+    ClientIdentifierCreationListener clientIdentifierCreationListener;
+
    View rootView;


View should be private even if this is not your change, please do it since we've spotted the problem in this review.

droidchef · 2017-08-07T11:36:43Z

.../src/main/java/com/mifos/mifosxdroid/online/clientidentifiers/ClientIdentifiersFragment.java

@@ -130,16 +132,34 @@ public void showUserInterface() {
    public void showClientIdentifiers(List<Identifier> identifiers) {
        this.identifiers = identifiers;
        identifierListAdapter.setIdentifiers(identifiers);
+        identifierListAdapter.notifyDataSetChanged();


This is a bad way of handling changes on a recycler view. I'd recommend reading Google's Documentation about RecyclerView.

Hint : You should almost never call notifyDataSetChanged() method on a RecyclerView's Adapter. There are other methods available for addition, modification, removal etc.

Also read about DiffUtils and try to use that here.

I am aware we use this bad approach all across the app but this is the time you learn and do it and set a precedence for the future devs so we can just point them to your code and they can learn from it.

I read about DiffUtils and if I understood it well, it's a good option to juxtapose two lists. In our case, however, we are sure of either 'adding' or 'removing' a single identifier. How about we have a method in the adapter to add (or remove the identifier) and then update the adapter accordingly? Or is DiffUtils a better option?

I agree with you ! DiffUtils is not relevant for our case. However I just wanted to you to know about it :) It is optimized for large operations in our case we won't have very large lists to play with. You can even copy new lists to old ones :P
However since we know about additions and deletions we can use more precise methods than just notifyDataSetChanged()

Makes sense ?

Yes. Used notifyItemInserted(position) and notifyItemRemoved(position) methods now and saved some bandwidth. :P

droidchef · 2017-08-07T11:38:07Z

.../src/main/java/com/mifos/mifosxdroid/online/clientidentifiers/ClientIdentifiersFragment.java

+
+    @Override
+    public void onClientIdentifierCreationFailure() {
+


This is Nasty !! 😡 An Empty Stub Method pushed sent in a PR that does nothing. WHY is it not implemented ?

Sorry! It was handled in the dialogfragment too. Nevertheless, I edited and implemented in a better way now.

droidchef · 2017-08-07T11:39:48Z

.../src/main/java/com/mifos/mifosxdroid/online/clientidentifiers/ClientIdentifiersFragment.java

        ll_error.setVisibility(View.VISIBLE);
        mNoIdentifierText.setText(getResources().getString(R.string.no_identifier_to_show));
        mNoIdentifierIcon.setImageResource(R.drawable.ic_assignment_turned_in_black_24dp);
    }

    @Override
+    public void onClientIdentifierCreationSuccess() {
+        loadIdentifiers();


Why do you want to retrieve all the identifiers again, when you create a new Identifier ? You have most of the data already that you POST, you'll get some stuff from the server in the response like ID or whatever that the server is responsible for generating. Why don't you just push that stuff into your list ?

Thanks! I never noticed this tbh. Implemented it this way.

droidchef · 2017-08-07T11:40:11Z

...in/java/com/mifos/mifosxdroid/dialogfragments/identifierdialog/IdentifierDialogFragment.java

@@ -187,4 +195,8 @@ public void onItemSelected(AdapterView<?> parent, View view, int position, long
    public void onNothingSelected(AdapterView<?> parent) {

    }
+
+    public void setOnClientIdentifierCreationListener(ClientIdentifierCreationListener listener) {


@Nullable annotation missing.

therajanmaurya · 2017-08-22T19:34:22Z

mifosng-android/src/main/java/com/mifos/api/datamanager/DataManagerClient.java

     */
-    public Observable<GenericResponse> createClientIdentifier(int clientId,
-                                                              IdentifierPayload identifierPayload) {
+    public Observable<IdentifierCreationResponse> createClientIdentifier(


@tarun0 don't need to create new Model, It's generic response that's why @droidchef made it. It is making more sense.

@therajanmaurya We are not fetching the identifiers list again. We are retrieving the created identifier's id from the response based on the discussion with @droidchef That's why I changed it.

@droidchef Will it be a better approach to check if the GenericResponse got resourceId and similar values that we want to check and then proceed accordingly? You once told about the naming and readability. I created the new model following those lines.

therajanmaurya · 2017-08-22T19:53:36Z

mifosng-android/src/main/java/com/mifos/api/services/ClientService.java

     */
    @POST(APIEndPoint.CLIENTS + "/{clientId}/identifiers")
-    Observable<GenericResponse> createClientIdentifier(@Path("clientId") int clientId,
-                                                       @Body IdentifierPayload identifierPayload);
+    Observable<IdentifierCreationResponse> createClientIdentifier(


same as above

therajanmaurya · 2017-08-28T20:27:26Z

@tarun0 Please fix the conflicts.

tarun0 force-pushed the fix658 branch 3 times, most recently from e83f15f to 11639a7 Compare August 7, 2017 10:45

tarun0 changed the title ~~fix: Update identifiers list on removing the only item~~ fix: Update identifiers list on adding or removing items Aug 7, 2017

droidchef requested changes Aug 7, 2017

View reviewed changes

tarun0 force-pushed the fix658 branch 3 times, most recently from 50455a2 to 877b44c Compare August 8, 2017 08:51

tarun0 mentioned this pull request Aug 17, 2017

Issues in client identifiers #768

Closed

therajanmaurya requested changes Aug 22, 2017

View reviewed changes

fix: Update client identifier list when identifier is added or removed

6421993

tarun0 force-pushed the fix658 branch from 877b44c to 6421993 Compare August 29, 2017 08:38

therajanmaurya approved these changes Mar 7, 2018

View reviewed changes

therajanmaurya merged commit 1cee4f4 into openMF:master Mar 7, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Update identifiers list on adding or removing items #743

fix: Update identifiers list on adding or removing items #743

tarun0 commented Aug 7, 2017 •

edited

Loading

tarun0 commented Aug 7, 2017

droidchef Aug 7, 2017

droidchef Aug 7, 2017

droidchef Aug 7, 2017

droidchef Aug 7, 2017

tarun0 Aug 8, 2017 •

edited

Loading

droidchef Aug 8, 2017

tarun0 Aug 8, 2017

droidchef Aug 7, 2017

tarun0 Aug 8, 2017 •

edited

Loading

droidchef Aug 7, 2017

tarun0 Aug 8, 2017

droidchef Aug 7, 2017

therajanmaurya Aug 22, 2017

tarun0 Aug 23, 2017

tarun0 Aug 23, 2017

therajanmaurya Aug 22, 2017

therajanmaurya commented Aug 28, 2017


		void onClientIdentifierCreationSuccess();

		void onClientIdentifierCreationFailure();

fix: Update identifiers list on adding or removing items #743

fix: Update identifiers list on adding or removing items #743

Conversation

tarun0 commented Aug 7, 2017 • edited Loading

tarun0 commented Aug 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tarun0 Aug 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tarun0 Aug 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

therajanmaurya commented Aug 28, 2017

tarun0 commented Aug 7, 2017 •

edited

Loading

tarun0 Aug 8, 2017 •

edited

Loading

tarun0 Aug 8, 2017 •

edited

Loading