Thumbs-up function 👍

mkeeda commented 4 years ago

Issue

close #429

Overview (Required)

Implement thumbs-up button like medium's crap
Thumbs-up counts are shared to all users
Only request to update the thumbs-up count if a particular timespan has passed

Links

-

Screenshot

Before	After

takahirom commented 4 years ago

👀

takahirom commented 4 years ago

I commented in this 🙏 https://github.com/DroidKaigi/conference-app-2020/issues/429#issuecomment-578519980

takahirom commented 4 years ago

I applied some fixes. Can you check it? 🙏 https://github.com/DroidKaigi/conference-app-2020/commit/4a2de2517c3f9534ca0f1d34283caeecab2fd9c1

takahirom commented 4 years ago

mkeeda commented 4 years ago

@takahirom Thank you for fix my code I checked your code, but I caught the permission error too. https://github.com/DroidKaigi/conference-app-2020/pull/626#discussion_r373835072 286d6bd debugging code is used by me.

Now, it is failure to retrieve shards and to create shards. I cannot access to thumbsup_counters collection, probably. Is my development enviroment some wrong?

I tried the simple code that retrieves documents of thumbsup_counters, but I cannot retrieve documents and catch the error.

code

FirebaseFirestore
    .getInstance()
    .collection("confsched/2020/sessions/155510/thumbsup_counters")
    .get()
    .addOnFailureListener {
        println(it)
    }
    .addOnSuccessListener {
        println(it.documents)
    }

error

2020-02-02 19:35:30.056 9913-9913/io.github.droidkaigi.confsched2020.debug E/AndroidRuntime: FATAL EXCEPTION: main Process: io.github.droidkaigi.confsched2020.debug, PID: 9913 com.google.android.gms.tasks.RuntimeExecutionException: com.google.firebase.firestore.FirebaseFirestoreException: PERMISSION_DENIED: Missing or insufficient permissions. at com.google.android.gms.tasks.zzu.getResult(Unknown Source:15) at io.github.droidkaigi.confsched2020.data.firestore.internal.FirestoreImpl$getFavoriteSessionIds$2.onComplete(FirestoreImpl.kt:54) at com.google.android.gms.tasks.zzj.run(Unknown Source:4) at android.os.Handler.handleCallback(Handler.java:883) at android.os.Handler.dispatchMessage(Handler.java:100) at android.os.Looper.loop(Looper.java:214) at android.app.ActivityThread.main(ActivityThread.java:7356) at java.lang.reflect.Method.invoke(Native Method) at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:492) at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:930) Caused by: com.google.firebase.firestore.FirebaseFirestoreException: PERMISSION_DENIED: Missing or insufficient permissions. at com.google.firebase.firestore.util.Util.exceptionFromStatus(com.google.firebase:firebase-firestore@@20.2.0:121) at com.google.firebase.firestore.core.EventManager.onError(com.google.firebase:firebase-firestore@@20.2.0:134) at com.google.firebase.firestore.core.SyncEngine.handleRejectedListen(com.google.firebase:firebase-firestore@@20.2.0:399) at com.google.firebase.firestore.core.FirestoreClient.handleRejectedListen(com.google.firebase:firebase-firestore@@20.2.0:287) at com.google.firebase.firestore.remote.RemoteStore.processTargetError(com.google.firebase:firebase-firestore@@20.2.0:555) at com.google.firebase.firestore.remote.RemoteStore.handleWatchChange(com.google.firebase:firebase-firestore@@20.2.0:436) at com.google.firebase.firestore.remote.RemoteStore.access$100(com.google.firebase:firebase-firestore@@20.2.0:53) at com.google.firebase.firestore.remote.RemoteStore$1.onWatchChange(com.google.firebase:firebase-firestore@@20.2.0:176) at com.google.firebase.firestore.remote.WatchStream.onNext(com.google.firebase:firebase-firestore@@20.2.0:108) at com.google.firebase.firestore.remote.WatchStream.onNext(com.google.firebase:firebase-firestore@@20.2.0:38) at com.google.firebase.firestore.remote.AbstractStream$StreamObserver.lambda$onNext$1(com.google.firebase:firebase-firestore@@20.2.0:119) at com.google.firebase.firestore.remote.AbstractStream$StreamObserver$$Lambda$2.run(Unknown Source:4) at com.google.firebase.firestore.remote.AbstractStream$CloseGuardedRunner.run(com.google.firebase:firebase-firestore@@20.2.0:67) at com.google.firebase.firestore.remote.AbstractStream$StreamObserver.onNext(com.google.firebase:firebase-firestore@@20.2.0:110) at com.google.firebase.firestore.remote.FirestoreChannel$1.onMessage(com.google.firebase:firebase-firestore@@20.2.0:120) at io.grpc.ForwardingClientCallListener.onMessage(ForwardingClientCallListener.java:33) at io.grpc.ForwardingClientCallListener.onMessage(ForwardingClientCallListener.java:33) at io.grpc.internal.ClientCallImpl$ClientStreamListenerImpl$1MessagesAvailable.runInContext(ClientCallImpl.java:563) at io.grpc.internal.ContextRunnable.run(ContextRunnable.java:37) at io.grpc.internal.SerializingExecutor.run(SerializingExecutor.java:123) at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:462) at java.util.concurrent.FutureTask.run(FutureTask.java:266) at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:301) at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1167) at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:641) at com.google.firebase.firestore.util.AsyncQueue$DelayedStartFactory.run(com.google.firebase:firebase-firestore@@20.2.0:205) at java.lang.Thread.run(Thread.java:919) Caused by: io.grpc.StatusException: PERMISSION_DENIED: Missing or insufficient permissions. 2020-02-02 19:35:30.056 9913-9913/io.github.droidkaigi.confsched2020.debug E/AndroidRuntime: at io.grpc.Status.asException(Status.java:541) at com.google.firebase.firestore.util.Util.exceptionFromStatus(com.google.firebase:firebase-firestore@@20.2.0:119) ... 26 more

takahirom commented 4 years ago

We haven't changed any rules since we created the issue. Perhaps you can create that collection, but you don't have permission to get it, so it is expected that isEmpty will be true. I am thinking about rule settings that can only get the size.

rules_version = '2';
service cloud.firestore {
  match /databases/{database}/documents {
    match /confsched/2020 {
...
      match /sessions/{sessionId} {
        match /{allSubcollections=**} {
          allow create
        }
        // Allow to increment only the 'shards' field and only by 1.
        match /thumbsup_counters/{counterId} {
          allow get;
          allow write: if request.resource.data.keys() == ["shards"]
                         && (resource == null || request.resource.data.shards ==
                         resource.data.shards + 1);
        }
      }
    }
  }
}

takahirom commented 4 years ago

I changed the rule 🙏

rules_version = '2';
service cloud.firestore {
  match /databases/{database}/documents {
    match /confsched/2020 {
...
      match /sessions/{sessionId} {
        match /{allSubcollections=**} {
          allow create
        }
        match /thumbsup_counters {
          allow read
          // Allow to increment only the 'shards' field and only by 1.
          match /{counterId} {
            allow write: if request.resource.data.keys() == ["shards"]
                           && (resource == null || request.resource.data.shards ==
                           resource.data.shards + 1);
          }
        }
      }
    }
  }
}

takahirom commented 4 years ago

I changed!

      match /sessions/{sessionId} {
        match /{allSubcollections=**} {
          allow create
        }
        match /thumbsup_counters/{counterId} {
            allow read;
              // Allow to increment only the 'shards' field and only by 1.
            allow write: if request.resource.data.keys() == ["shards"]
                           && (resource == null || request.resource.data.shards ==
                           resource.data.shards + 1);
        }
      }

mkeeda commented 4 years ago

The code fail to create shards... I misunderstood that I successed to create shards...? I will find out the cause tomorrow.

takahirom commented 4 years ago

Maybe even if there is a value, I think that isEmpty is true. Personally, I think it's a good idea to check if there are 9 counters so that the process can die during creation. Or it might be better to use something like a transaction https://firebase.google.com/docs/firestore/manage-data/transactions

takahirom commented 4 years ago

I changed the rule 🙏

                           request.resource.data.shards > resource.data.shards &&
                           request.resource.data.shards < resource.data.shards + 51

mkeeda commented 4 years ago

TODO

[x] Fix tests
[ ] Implement error handling when increment thumbs-up count is failure

takahirom commented 4 years ago

I implemented debounce logic 🙏 https://github.com/DroidKaigi/conference-app-2020/commit/be331b0421f750d8ade541c5c1be3d3b36ef8dd5

mkeeda commented 4 years ago

@takahirom I moved the debouce processing ViewModel, and removed the code of checking index. Probably, the debounce processing is a bussiness logic, soFirestore should not have it. 67e699a

And I want to show own incrementd count like medium. So I separated increment flow from total thumbs up count flow. https://github.com/DroidKaigi/conference-app-2020/issues/429#issuecomment-582482685

takahirom commented 4 years ago

:eyes:

takahirom commented 4 years ago

As for business logic, it is difficult because there is a possibility that duplicate logic will be created by bringing it to the ViewModel. However, if you want to have incrementThumbsUpCount in UiModel, I think it is good.

takahirom commented 4 years ago

For now, let's proceed with the implementation of displaying incrementThumbsUpCount! 👍 この方針のまま、実装を進めていいと思います！

takahirom commented 4 years ago

@mkeeda Any progress...? 🙇

mkeeda commented 4 years ago

@takahirom Sorry, I made you wait 🙇 🙇 🙇 I implemented popup animation! thumbsup

TODO

[x] Pass CircleCI checking
[x] Implement error handling when increment thumbs-up count is failure

takahirom commented 4 years ago

👀

mkeeda commented 4 years ago

The popup animation is too moved? It is little hard to watch a incremented count for me.

takahirom commented 4 years ago

@mkeeda Thanks as always 🙏 Can you take time today? What are the remaining tasks now?

mkeeda commented 4 years ago

@takahirom Sorry, I’m almost done! I will finish tasks today. The remaining tasks ↓

[x] Cherry-pick 70f7d25 (Conflicting my changes...)
[x] Refactor animation functions
[x] Remove debug log (I want to remove ⭐print)

I will write overviews at top of this PR when I finished all tasks.

takahirom commented 4 years ago

Thanks! I want to merge this today!! 👍

takahirom commented 4 years ago

Can you delete WIP? 😄

mkeeda commented 4 years ago

Yes! I'm writing overviews now!

jmatsu-bot commented 4 years ago

Your apk has been deployed to https://deploygate.com/distributions/1f10badbf5012479d46a7dffcc44c18b5a4c3beb. Anyone can try your changes via the link.

Generated by :no_entry_sign: Danger

jmatsu-bot commented 4 years ago

No error was reported but at least one warning was found.

Generated by :no_entry_sign: Danger

mkeeda commented 4 years ago

@takahirom All tasks is done! Please review again 🙏 🙏 🙏

takahirom commented 4 years ago

LGTM 👍👍👍👍

DroidKaigi / conference-app-2020