dotcom-rendering icon indicating copy to clipboard operation
dotcom-rendering copied to clipboard

Implement Recommend on Android

Open mxdvl opened this issue 1 year ago • 1 comments

mxdvl avatar Feb 21 '24 10:02 mxdvl

https://github.com/guardian/android-news-app/pull/10044

alinaboghiu avatar Feb 22 '24 10:02 alinaboghiu

Update after a discussion with @robksmith, @EnochHankombo92 and @ab-gnm (meeting notes here) and some thoughts after chatting with @georgeblahblah:

AsyncIface not working in the Android integration yet

In:

  • https://github.com/guardian/android-news-app/pull/10044

we tried to use the AsyncIface but as was initially suggested by @ab-gnm in the review this is not working in the Android integration yet.

@robksmith and @ioannakok verified this:

  • We tried to run #10044 locally but only the UI gets updated and result does not persist.
  • Added logging in a branch and noticed recommend does not get called
  • Replaced AsyncIface with Iface and saw the logs from recommend coming through

@ab-gnm has a draft PR to get AsyncIface working but this work has not been completed yet:

  • https://github.com/guardian/android-news-app/pull/9904

Current options (until AsyncIface works) and consequences

  • Use Iface like NotificationsImpl is doing at the moment for the follow button functionality
  • Downside is the follow method is currently returning true no matter what the real outcome of the request is.
  • An alternative possibility was mentioned re using the main thread for Bridget-Android communication but this sounds risky.

Thoughts / Questions

  • Could we use Iface temporarily in order to get only the recommend comment functionality to prod?
  • This doesn't look like an option though for posting / replying to a comment & adding a username. Possible workaround suggested by the Android team until the AsyncIface works: Store comments in a local cache and try to repeat posting them. This will mean possible delays from when a user posts a comment until the action is actually completed.
  • Even if we used the local cache option temporarily there are many different types of errors the discussion api could return. Could the local cache option handle this?
  • If we decide to wait until the AsyncIface works in order to implement posting / replying to a comment & adding a username, does this mean this work gets blocked for iOS as well?

ioannakok avatar Mar 12 '24 17:03 ioannakok

We had a team discussion with @abeddow91, @daniel-guardian, @georgeblahblah and @arelra about this. Notes here

It looks like we have three options at the moment:

  1. Proceed with the features of discussion we think it’s ok to optimistically update the UI, e.g. recommend
  2. Re-write bridget models to request authentication tokens and do all the async communication in the web layer. This would change the current flow.
  3. Mixed release: Implement everything on iOS, update MAPI and leave Android one version behind. We can combine this with option 1 by doing first releases of discussion features where we can optimistically update the UI (e.g. recommend)

Before we decide which option, we would like to get PMs / EMs to discuss when the async work can be planned / prioritised in their current stream of work. The goal would be to get a better understanding of the timeline not so much to do it asap.

ioannakok avatar Mar 13 '24 13:03 ioannakok

As a final check to verify that the AsyncIface is not working @georgeblahblah and I did the following test:

  1. Upvoted a comment in the emulator
  2. Upvoted the same comment in the Web by being signed in as the same user. We didn't get a "CAN'T_RECOMMEND_SAME_COMMENT_TWICE" error as is the expected behaviour which verifies it doesn't work.
  3. Upvoted another comment in the Web twice and got the expected error.

PS. Had to split the test in multiple videos due to size.

https://github.com/guardian/dotcom-rendering/assets/19683595/12124246-e620-4697-b0f7-862e4f9cecf3

https://github.com/guardian/dotcom-rendering/assets/19683595/3fc9b184-3228-4271-9d7f-b0d4ea2d9aaf

https://github.com/guardian/dotcom-rendering/assets/19683595/41ffbc9a-0988-450f-bd9e-69d264f08607

ioannakok avatar Mar 13 '24 17:03 ioannakok

Closing as this is ready to go to beta. We can reopen if there are any more issues.

georgeblahblah avatar May 17 '24 13:05 georgeblahblah