[NEW] Sync deleted messages #2617

luismachado · 2019-03-29T10:50:17Z

Checks for deleted messages every time messages are fetched (direct messages)

For now I've only implemented it for direct messages (and it's lacking optimisation), but would this be a viable way to sync (i.e. remove from the device) deleted messages?

@RocketChat/ios

Closes #2208

…essages)

houndci-bot · 2019-03-29T10:50:20Z

Rocket.Chat/API/Requests/Room/RoomDeletedMessagesRequest.swift

+    let roomId: String?
+    let since: Date?
+
+    // How to improve this default date? We only want deleted messages as far as the older message we have on cache! (older ones won't be returned)


Line Length Violation: Line should be 120 characters or less: currently 147 characters (line_length)

houndci-bot · 2019-03-29T10:50:20Z

Rocket.Chat/API/Clients/SubscriptionsClient.swift

+
+    private func loadDeletedMessages(subscription: Subscription, completion: (([String]) -> Void)? = nil) {
+
+        // TODO: Save latest date? Older messages may have been deleted so we probably should always check up to the oldest


Line Length Violation: Line should be 120 characters or less: currently 123 characters (line_length)
Todo Violation: TODOs should be resolved (Save latest date? Older messag...). (todo)

CLAassistant · 2019-03-29T10:50:24Z

All committers have signed the CLA.

CLAassistant · 2019-03-29T10:50:24Z

Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

codecov · 2019-03-29T11:36:17Z

Codecov Report

Merging #2617 into develop will increase coverage by 0.16%.
The diff coverage is 66.66%.

@@             Coverage Diff             @@
##           develop    #2617      +/-   ##
===========================================
+ Coverage    26.15%   26.32%   +0.16%     
===========================================
  Files          463      464       +1     
  Lines        17216    17252      +36     
===========================================
+ Hits          4503     4541      +38     
+ Misses       12713    12711       -2

Impacted Files	Coverage Δ
...API/Requests/Room/RoomDeletedMessagesRequest.swift	`55% <55%> (ø)`
Rocket.Chat/API/Clients/SubscriptionsClient.swift	`67.14% <77.27%> (+0.13%)`	⬆️
...anagers/Model/AuthManager/AuthManagerRecover.swift	`36.36% <0%> (-60.61%)`	⬇️
Rocket.Chat/Extensions/Models/AuthExtensions.swift	`66.66% <0%> (-33.34%)`	⬇️
...Chat/Models/Subscription/SubscriptionQueries.swift	`70% <0%> (-30%)`	⬇️
Rocket.Chat/Managers/PushManager.swift	`24.13% <0%> (-10.35%)`	⬇️
Rocket.Chat/Managers/AppManager.swift	`36.04% <0%> (-4.07%)`	⬇️
Rocket.Chat/API/APIRequest.swift	`97.29% <0%> (+2.7%)`	⬆️
...Chat/Controllers/Chat/MessagesViewController.swift	`34.25% <0%> (+2.75%)`	⬆️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ecad166...c4d7b22. Read the comment docs.

rafaelks · 2019-03-29T13:33:44Z

@luismachado Amazing, thanks for the PR! Gonna review it for release 3.5, we're wrapping up 3.4 right now. 👏

rafaelks · 2019-04-02T12:14:14Z

@luismachado This PR is missing your signature of CLA, could you do that please?

rafaelks

Thanks for the PR @luismachado, most of it looks very good, but the way you're doing the request is not the way it should be based on the API. Here's how it should work:

Every time the user opens a subscription, we need to request the latest deleted messages based on the last time we requested them, so for example: if we requested them on time 1000, next time we'll pass 1000 as since parameter. If now the time is 2000, we will update the 1000 value to 2000 for the next request (next time user opens the subscription). Got it?

We don't need to request it every time we request the history of messages;
It needs to have a since parameter valid, not 0. This will prevent us from downloading very old data from the server.

Let me know if that makes sense to you please.

Thank you!

luismachado · 2019-04-03T08:46:01Z

Hello @rafaelks,

I do understand what you're saying (and the fix definitely needs some improvements) but I'm not sure I would 100% agree with you (or maybe I didn't understand you fully :P).

What I was thinking would be to grab the oldest message timestamp (I think this is already done) and then use that timestamp to fetch the deleted list.
I'm not sure that we can avoid fetching deleted info that was already fetched because the users can delete any message, old or new, so we have to make sure that we always have the most recent info on deleted messages for the our current scope (i.e. the messages we have on cache).

We don't need to request it every time we request the history of messages
In which situations could we avoid it? Maybe we could only call the endpoint for the first time the history is requested (i.e, when the messages controller is opened). I'm thinking this because (and please correct me if I'm wrong) if a (unfetched) message was deleted in the meanwhile it won't be returned by the api.

rafaelks · 2019-04-03T12:08:59Z

What I was thinking would be to grab the oldest message timestamp (I think this is already done) and then use that timestamp to fetch the deleted list.

I'm not sure that we can avoid fetching deleted info that was already fetched because the users can delete any message, old or new, so we have to make sure that we always have the most recent info on deleted messages for the our current scope (i.e. the messages we have on cache).

AFAIK the date you pass as a parameter is not the date of the message, but the date that the "updates" happen, so if a message from 5 days ago was deleted today and you fetch the API with the "since" from today, you will get the deleted message information, you know?

luismachado · 2019-04-03T16:13:19Z

Ah that makes much more sense :) I'm going to go over it then but I'll probably only have news next week.

…

On Wed, Apr 3, 2019 at 1:09 PM Rafael Kellermann Streit < ***@***.***> wrote: What I was thinking would be to grab the oldest message timestamp (I think this is already done) and then use that timestamp to fetch the deleted list. I'm not sure that we can avoid fetching deleted info that was already fetched because the users can delete any message, old or new, so we have to make sure that we always have the most recent info on deleted messages for the our current scope (i.e. the messages we have on cache). AFAIK the date you pass as a parameter is not the date of the message, but the date that the "updates" happen, so if a message from 5 days ago was deleted today and you fetch the API with the "since" from today, you will get the deleted message information, you know? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2617 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHtxo4AWWK1PRd5jWBhFIv6m-cxkp1gCks5vdJndgaJpZM4cSIC7> .

-- *Luís Machado* *Software Engineer / iOS Developer - Website <http://lfmdevelopment.com>*

Checks for deleted messages every time messages are fetched (direct m…

c4d7b22

…essages)

houndci-bot reviewed Mar 29, 2019

View reviewed changes

rafaelks self-assigned this Mar 29, 2019

rafaelks self-requested a review March 29, 2019 13:33

rafaelks added the 🎉 feature label Mar 29, 2019

rafaelks added this to the 3.5.0 milestone Mar 29, 2019

rafaelks changed the title ~~Sync deleted messages~~ [NEW] Sync deleted messages Mar 29, 2019

rafaelks suggested changes Apr 2, 2019

View reviewed changes

rafaelks added the 🚨 needs changes label Apr 2, 2019

rafaelks modified the milestones: 3.4.1, 3.5.0 Apr 5, 2019

rafaelks modified the milestones: 3.5.0, 3.6.0 May 8, 2019

AdamUncao mentioned this pull request Jul 15, 2019

[NEW] message deletion syncing #2738

Closed

rafaelks removed their assignment Aug 1, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NEW] Sync deleted messages #2617

[NEW] Sync deleted messages #2617

luismachado commented Mar 29, 2019

houndci-bot Mar 29, 2019

houndci-bot Mar 29, 2019

CLAassistant commented Mar 29, 2019 •

edited

Loading

CLAassistant commented Mar 29, 2019

codecov bot commented Mar 29, 2019 •

edited

Loading

rafaelks commented Mar 29, 2019

rafaelks commented Apr 2, 2019

rafaelks left a comment

luismachado commented Apr 3, 2019

rafaelks commented Apr 3, 2019

luismachado commented Apr 3, 2019 via email


		private func loadDeletedMessages(subscription: Subscription, completion: (([String]) -> Void)? = nil) {

		// TODO: Save latest date? Older messages may have been deleted so we probably should always check up to the oldest

[NEW] Sync deleted messages #2617

Are you sure you want to change the base?

[NEW] Sync deleted messages #2617

Conversation

luismachado commented Mar 29, 2019

houndci-bot Mar 29, 2019

Choose a reason for hiding this comment

houndci-bot Mar 29, 2019

Choose a reason for hiding this comment

CLAassistant commented Mar 29, 2019 • edited Loading

CLAassistant commented Mar 29, 2019

codecov bot commented Mar 29, 2019 • edited Loading

Codecov Report

rafaelks commented Mar 29, 2019

rafaelks commented Apr 2, 2019

rafaelks left a comment

Choose a reason for hiding this comment

luismachado commented Apr 3, 2019

rafaelks commented Apr 3, 2019

luismachado commented Apr 3, 2019 via email

CLAassistant commented Mar 29, 2019 •

edited

Loading

codecov bot commented Mar 29, 2019 •

edited

Loading