dotcom-rendering
dotcom-rendering copied to clipboard
Implement Recommend on Android
https://github.com/guardian/android-news-app/pull/10044
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
withIface
and saw the logs fromrecommend
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
likeNotificationsImpl
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 therecommend
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?
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:
- Proceed with the features of discussion we think it’s ok to optimistically update the UI, e.g. recommend
- Re-write bridget models to request authentication tokens and do all the async communication in the web layer. This would change the current flow.
- 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.
As a final check to verify that the AsyncIface
is not working @georgeblahblah and I did the following test:
- Upvoted a comment in the emulator
- 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.
- 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
Closing as this is ready to go to beta. We can reopen if there are any more issues.