Rocket.Chat.ReactNative icon indicating copy to clipboard operation
Rocket.Chat.ReactNative copied to clipboard

feat: automatically send failed messages on app reconnect

Open jsathu07 opened this issue 1 year ago • 16 comments

Proposed changes

The feature to automatically send failed messages when app reconnects

Issue(s)

closes #4471

How to test or reproduce

Screenshots

After

https://github.com/RocketChat/Rocket.Chat.ReactNative/assets/61021881/05a08fb1-021b-4715-85c0-50dcfc538250

Types of changes

  • [ ] Bugfix (non-breaking change which fixes an issue)
  • [ ] Improvement (non-breaking change which improves a current function)
  • [x] New feature (non-breaking change which adds functionality)
  • [ ] Documentation update (if none of the other choices apply)

Checklist

  • [x] I have read the CONTRIBUTING doc
  • [x] I have signed the CLA
  • [x] Lint and unit tests pass locally with my changes
  • [ ] I have added tests that prove my fix is effective or that my feature works (if applicable)
  • [ ] I have added necessary documentation (if applicable)
  • [ ] Any dependent changes have been merged and published in downstream modules

Further comments

jsathu07 avatar Dec 26 '23 21:12 jsathu07

@diegolmello I have implemented the feature. Kindly review the code at your convenience sir, Thank you.

jsathu07 avatar Dec 26 '23 21:12 jsathu07

@GleidsonDaniel Can you check my pull request when you have time? Thank you sir

jsathu07 avatar Jan 12 '24 15:01 jsathu07

@diegolmello Should I also consider resending attachments?

jsathu07 avatar Jan 13 '24 06:01 jsathu07

@diegolmello Should I also consider resending attachments?

I don't think so, it's better to deal with it in another PR and another function. Looking at the method, I don't think there's anything wrong, but @diegolmello has more knowledge of all the APP's flows, so I leave this double check to him.

dnlsilva avatar Jan 15 '24 16:01 dnlsilva

@diegolmello Should I also consider resending attachments?

@diegolmello Should I also consider resending attachments?

I don't think so, it's better to deal with it in another PR and another function. Looking at the method, I don't think there's anything wrong, but @diegolmello has more knowledge of all the APP's flows, so I leave this double check to him.

@jsathu07 I'd do it on the same function, because of where it's called and timestamps. I think it's better to do it on this PR as well, because It's going to look weird if it resends only half of failing messages.

diegolmello avatar Jan 15 '24 16:01 diegolmello

@diegolmello Should I also consider resending attachments?

@diegolmello Should I also consider resending attachments?

I don't think so, it's better to deal with it in another PR and another function. Looking at the method, I don't think there's anything wrong, but @diegolmello has more knowledge of all the APP's flows, so I leave this double check to him.

@jsathu07 I'd do it on the same function, because of where it's called and timestamps. I think it's better to do it on this PR as well, because It's going to look weird if it resends only half of failing messages.

Sure sir I'll implement that too

jsathu07 avatar Jan 15 '24 18:01 jsathu07

@diegolmello How the app should behave if it needs to resend msgs and attachments (doesn't have ts)

jsathu07 avatar Jan 20 '24 15:01 jsathu07

@jsathu07 Can you elaborate your question?

diegolmello avatar Jan 22 '24 18:01 diegolmello

@diegolmello Sir, if someone sends text messages and uploads images, the current code doesn't provide information about when the uploads were started. To address this, when resending messages, should I initiate resending of all uploads first and then the messages or do it the other way around. Best option is to include a "created at" field in the uploads database, sort all messages based on timestamps, and then initiate the resending process. Please share your thoughts whether I can modify the db

jsathu07 avatar Jan 22 '24 18:01 jsathu07

@diegolmello Sir, if someone sends text messages and uploads images, the current code doesn't provide information about when the uploads were started. To address this, when resending messages, should I initiate resending of all uploads first and then the messages or do it the other way around. Best option is to include a "created at" field in the uploads database, sort all messages based on timestamps, and then initiate the resending process. Please share your thoughts whether I can modify the db

@jsathu07 If what's missing is a date column to sort, I say we create Uploads.ts column to do it. I didn't find any created_at, so I think ts is better just for the sake of consistency. Don't forget to add a migration.

diegolmello avatar Jan 22 '24 19:01 diegolmello

@diegolmello Sir, if user has any attachments which needs to be resend, which were initiated before migration how should I handle it? Should I leave it without resending so that user can initiate resending if they want

jsathu07 avatar Jan 23 '24 11:01 jsathu07

@diegolmello Sir, if user has any attachments which needs to be resend, which were initiated before migration how should I handle it? Should I leave it without resending so that user can initiate resending if they want

@jsathu07 Yes, you can do it like that.

diegolmello avatar Jan 23 '24 12:01 diegolmello

@diegolmello, I've completed the feature sir. Could you review it when you have time, Thanks

jsathu07 avatar Jan 23 '24 16:01 jsathu07

@diegolmello tell me if there is anything I can do to improve or change in this pr sir. Thanks

jsathu07 avatar Feb 08 '24 16:02 jsathu07

@jsathu07 In a few days. We have other priorities rn.

diegolmello avatar Feb 08 '24 17:02 diegolmello