Revert "Merge pull request #4085 from Expensify/chirag-revert-3979" #4133
Conversation
…t-3979" This reverts commit c2f92b6.
|
Hmm, I don't get why we show "[Comment deleted]" when there are previous messages. If it's the first and only message then I think "[Comment deleted]" is fine but otherwise couldn't we show the previous message in the LHN @dklymenk? I briefly checked the code and it looks like we refer |
I don't think showing the actual last message during initial load is an issue, however updating it in real time is. I have had submitted 2 or 3 proposals in total with an implementation that works, but no matter what you do you are basically required to fetch the next last message from onyx storage every time you receive a Pusher update. It has been determined that if we cannot implement this without onyx, we shouldn't do it and instead stick to Also the original PR #3979 has been approved and merged. The only reason it was reverted in the first place, as you probably remember, is because in combination with #3737 it resulted in |
|
Yup, i think your solution here is totally good. I guess I was thinking of another improvement that could be done on the backend to make the user experience even better which adds to the code you have here. But I guess that's for another time. |
chiragsalian
changed the title
|
Hold is gold here. #5571 can severely affect the outcome of this PR. |
|
I'm pretty sure the end solution for #5571 won't impact this PR, this should be good to go after merging in master |
|
Oops sorry if this is stalled due to my comment. It was pretty old. Yup, we are good to go... |
mallenexpensify
changed the title
|
Per @parasharrajat and @NikkiWines comment above, I removed |
|
Oh, sorry. I completely forgot that you were waiting for my signal on hold removal. No I am not working on resolving that e2e test issue. It came up after I fixed a conflict 3 weeks ago. It looks like some package wasn't fetched properly during installation or something. My changes don't touch anything in iOS folder, so I am pretty sure that error has nothing to do with this PR. Please let me know If you want me do anything. I just want these issues get closed already. Once again I am sorry for not writing here about hold removal. I honestly thought you were all busy or something. |
|
@dklymenk you might try merging main into your branch and see if that resolves the e2e issue. This branch is 662 commits behind Expensify:main, so it's a good idea to merge it even if it doesn't end up fixing the tests. |
|
Merged main into the branch. Tested the changes. Still looking good. The e2e test seems to have passed the stage it failed at last time. Hopefully, it finishes successfully and we get to merge this. |
|
@chiragsalian After merging main all tests have passed. The PR is ready for review once again. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by @chiragsalian in version: 1.1.15-18 🚀
|
|
🚀 Deployed to production by @roryabraham in version: 1.1.16-10 🚀
|
This reverts commit c2f92b6.
@chiragsalian this a PR related to our discussion here #3979 (comment)
Details
Fixed Issues
$ #3743
$ #3293
Tests / QA Steps
Tested On
Screenshots
Web
3747-3293-web.mp4
Mobile Web
Simulator.Screen.Recording.-.iPhone.8.-.2021-07-12.at.15.19.55.mp4
Desktop
Screen.Recording.2021-07-12.at.15.22.22.mov
iOS
Simulator.Screen.Recording.-.iPhone.12.-.2021-07-12.at.15.28.56.mp4
Android
3747-3293-android.mp4