Closed Bug 954118 Opened 10 years ago Closed 10 years ago

Handle deleting messages on twitter

Categories

(Chat Core :: Twitter, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: florian, Assigned: clokep)

References

Details

Attachments

(1 file, 5 obsolete files)

*** Original post on bio 683 at 2011-02-04 13:46:00 UTC ***

We should show which messages have been deleted (maybe gray them out in the timeline), and we should offer the user the ability to delete his own messages.
Blocks: 954035
*** Original post on bio 683 at 2011-10-27 00:57:09 UTC ***

(In reply to comment #0)
> We should show which messages have been deleted (maybe gray them out in the
> timeline),
I assume there's no support for something like this right now? Would we want purpleIMessage to have a "revoked" or "deleted" flag?

> and we should offer the user the ability to delete his own messages.
I assume we'd add this via the context menu?
Attached patch Adds a delete action (obsolete) — Splinter Review
*** Original post on bio 683 as attmnt 950 at 2011-10-27 01:25:00 UTC ***

This adds a delete action to the context menu which seems to work. You don't get any feedback though...it seems to just send the same tweet back to you (without any flags saying it's deleted, etc.), so I'm not sure how we should handle this.
Attachment #8352692 - Flags: review?(florian)
Assignee: nobody → clokep
Status: NEW → ASSIGNED
Component: General → Twitter
*** Original post on bio 683 at 2011-10-27 12:03:27 UTC ***

(In reply to comment #1)
> (In reply to comment #0)
> > We should show which messages have been deleted (maybe gray them out in the
> > timeline),
> I assume there's no support for something like this right now?

Right.

> Would we want purpleIMessage to have a "revoked" or "deleted" flag?

Maybe. It's not fully clear to me how the conversation view could be updated to reflect this change (or maybe this depends on bug 953743 (bio 300)).

> > and we should offer the user the ability to delete his own messages.
> I assume we'd add this via the context menu?

Yes.

(In reply to comment #2)

> You don't
> get any feedback though...it seems to just send the same tweet back to you

That's what the documentation (https://dev.twitter.com/docs/api/1/post/statuses/destroy/%3Aid) says, so I guess if we receive the same tweet, we can just assume the delete has been successful.

If you want a way to display that in the conversation, maybe redisplay the tweet with "Deleted: " prepended? And maybe the text of the tweet in <strike> (I'm not sure that would pass the markup filters we have, but I guess that can be fixed)?
*** Original post on bio 683 at 2011-10-27 12:14:44 UTC ***

(In reply to comment #3)
> If you want a way to display that in the conversation, maybe redisplay the
> tweet with "Deleted: " prepended? And maybe the text of the tweet in <strike>
> (I'm not sure that would pass the markup filters we have, but I guess that can
> be fixed)?
Perhaps this should be in a system message (and we wouldn't need to put the strike then)? I'll change the callback method and see what I can do.
*** Original post on bio 683 at 2011-10-27 12:31:52 UTC ***

(In reply to comment #4)

> Perhaps this should be in a system message (and we wouldn't need to put the
> strike then)?

That works too I guess :).
Comment on attachment 8352692 [details] [diff] [review]
Adds a delete action

*** Original change on bio 683 attmnt 950 at 2011-10-27 12:34:30 UTC ***

>diff --git a/purple/purplexpcom/src/twitter.js b/purple/purplexpcom/src/twitter.js

>   onSentCallback: function(aData) {
>+    Cu.reportError(aData);

Is this change intentional?

>+  destroy: function(aTweet, aOnSent, aOnError, aThis) {
>+    let url =
>+      "1/statuses/destroy/" + aTweet.id_str + ".json?include_entities=1";
>+    this.signAndSend(url, null, [], aOnSent, aOnError, aThis);

This isn't the right callback, as you noted in comment 4.
Attachment #8352692 - Flags: review?(florian) → review-
*** Original post on bio 683 at 2011-11-01 00:40:47 UTC ***

Something else to consider is that if we delete our newest tweet, we should update the topic to the tweet before that.
Attached patch Delete action v2 (obsolete) — Splinter Review
*** Original post on bio 683 as attmnt 970 at 2011-11-03 01:57:00 UTC ***

This removes my debug statement and adds a system message saying you deleted a tweet (followed by the contents of the tweet). I refactored the code that pulls the entities, etc. out of a tweet so I could reuse it for the system message (it didn't seem to make sense to me to show a tweet without this information, even if it's in a system message).
Attachment #8352712 - Flags: review?(florian)
Comment on attachment 8352692 [details] [diff] [review]
Adds a delete action

*** Original change on bio 683 attmnt 950 at 2011-11-03 01:57:15 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352692 - Attachment is obsolete: true
Comment on attachment 8352712 [details] [diff] [review]
Delete action v2

*** Original change on bio 683 attmnt 970 at 2011-11-10 00:10:44 UTC ***

I had to spend some time to unbitrot this patch before trying it. Too bad I've noticed only after the fact that it clearly wasn't going to be r+ :-(.

>diff --git a/chat/locales/en-US/twitter.properties b/chat/locales/en-US/twitter.properties

>+#  %S will be replaced by the tweet in reference.

I think this can be phrase a bit better ("tweet in reference" doesn't mean much to me).

>+event.deleted=You have deleted your tweet: "%S".

Maybe "You have deleted *this* tweet:" rather than "your tweet"?

>diff --git a/chat/protocols/twitter/twitter.js b/chat/protocols/twitter/twitter.js
>--- a/chat/protocols/twitter/twitter.js
>+++ b/chat/protocols/twitter/twitter.js
>@@ -90,16 +90,22 @@ Tweet.prototype = {
>       );
>       let friend = account.isFriend(this._tweet.user);
>       if (friend !== null) {
>         let action = friend ? "stopFollowing" : "follow";
>         let screenName = this._tweet.user.screen_name;
>         actions.push(new Action(_("action." + action, screenName),
>                                 function() { account[action](screenName); }));
>       }
>+    } else if (this.outgoing) {

Shouldn't we also check that the tweet hasn't been deleted already? Deleting a tweet twice causes an error ("no status for this ID") from the twitter server.

>+      actions.push(
>+        new Action(_("action.delete"), function() {
>+          this.conversation.destroy(this._tweet);

I wonder if we shouldn't move that destroy method to the tweet object, rather than the conversation.


>@@ -164,30 +177,45 @@ Conversation.prototype = {

>+  onDestroyCallback: function(aData) {
>+    let tweet = JSON.parse(aData);
>+    // When unsuccessful an error is returned.
>+    if (tweet.error)
>+      throw tweet.error;
>+
>+    // When successful, the tweet is returned.
>+    let [text, name] = this.account.timeline.parseTweet(tweet);
>+    Cu.reportError(text + "\n" + name);
>+    text = _("event.deleted", text);
>+    Cu.reportError(text);

Comment 8 says "This removes my debug statement". :(

>+    // Create a new system message saying the tweet has been deleted.
>+    (new Tweet(tweet, name, text, {system: true})).conversation = this;

Why a new Tweet rather than just this.systemMessage? The Tweet object in this case gives us the "Reply" and "Copy Link to Tweet" actions. I don't think we want them.

I'll attach the unbitrotted patch (with also some changes to the .properties file).
Attachment #8352712 - Flags: review?(florian) → review-
*** Original post on bio 683 as attmnt 983 at 2011-11-10 00:14:00 UTC ***

(In reply to comment #9)

> >diff --git a/chat/protocols/twitter/twitter.js b/chat/protocols/twitter/twitter.js

> >       }
> >+    } else if (this.outgoing) {

Also, coding style nit: always a new line before the else keyword.

> I'll attach the unbitrotted patch (with also some changes to the .properties
> file).

Done.
Comment on attachment 8352712 [details] [diff] [review]
Delete action v2

*** Original change on bio 683 attmnt 970 at 2011-11-10 00:14:50 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352712 - Attachment is obsolete: true
Attached patch Delete action v3 (obsolete) — Splinter Review
*** Original post on bio 683 as attmnt 984 at 2011-11-10 02:05:00 UTC ***

Switching it to the Tweet simplified the code a lot! Good idea.
Attachment #8352725 - Flags: review?(florian)
Comment on attachment 8352724 [details] [diff] [review]
Attachment 950 [details] without the bitrot

*** Original change on bio 683 attmnt 983 at 2011-11-10 02:05:45 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352724 - Attachment is obsolete: true
*** Original post on bio 683 at 2011-11-10 11:04:05 UTC ***

(In reply to comment #9)

> >+    } else if (this.outgoing) {
> 
> Shouldn't we also check that the tweet hasn't been deleted already? Deleting a
> tweet twice causes an error ("no status for this ID") from the twitter server.

It seems you have ignored this comment :'(.

The point of moving the conversation.destroy method into tweet.destroy was in part because it makes it easy to store a this._deleted = true value in the object after the success callback is called.

> >+      actions.push(
> >+        new Action(_("action.delete"), function() {
> >+          this.conversation.destroy(this._tweet);
> 
> I wonder if we shouldn't move that destroy method to the tweet object, rather
> than the conversation.

The part in the account object was at the right place. It's the account that signs and sends the request, with the account's credentials.
Comment on attachment 8352725 [details] [diff] [review]
Delete action v3

*** Original change on bio 683 attmnt 984 at 2011-11-10 11:15:30 UTC ***

See comment 12.

>diff --git a/chat/protocols/twitter/twitter.js b/chat/protocols/twitter/twitter.js

>+  destroy: function(aTweet) {
>+    let onErrorCallback = function(aException, aData) {
>+      this.conversation
>+          .systemMessage(_("error.delete", this.conversation._parseError(aData),
>+                           aTweet.text), true);
>+    };

Why isn't this onErrorCallback function not a method? Or why aren't the error and success callback implemented in the same way?

>+  onDestroyCallback: function(aData) {
>+    let tweet = JSON.parse(aData);
>+    // When unsuccessful an error is returned.
>+    if (tweet.error)

Isn't this accessing an undefined value if there's no error?

>+    // When successful, the tweet is returned.
>+    let [text, name] = this.conversation._account.timeline.parseTweet(tweet);

How is this.conversation._account.timeline different from this.conversation?

Or did you do this to deal with the case where the user has closed the timeline conversation before the feedback message arrives?
If so, I would prefer something like |if (this.conversation.closed (or whichever way we can test if the conversation was closed)) /* code to reopen */;  /* code to display the message */.

But should we really reopen the conversation only for a system message? In case of an error maybe, for success... I'm not sure.
Attachment #8352725 - Flags: review?(florian) → review-
Attached patch Delete action v4 (obsolete) — Splinter Review
*** Original post on bio 683 as attmnt 986 at 2011-11-11 00:36:00 UTC ***

I've added a _deleted flag, this is marked as true immediately upon a user clicking the context menu action. If an error in the connection occurs or if the connection is successful but an error state is returned we again mark the tweet as NOT deleted.

I've moved the part in the account back.

The onErrorCallback was an inline function as that's how it was in the code I copied.  It's now a separate function.

I'm now check if the error property exists and I'm using the simple "this.conversation" property.
Attachment #8352727 - Flags: review?(florian)
Comment on attachment 8352725 [details] [diff] [review]
Delete action v3

*** Original change on bio 683 attmnt 984 at 2011-11-11 00:36:28 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352725 - Attachment is obsolete: true
Comment on attachment 8352727 [details] [diff] [review]
Delete action v4

*** Original change on bio 683 attmnt 986 at 2011-12-13 21:30:28 UTC ***

>diff --git a/chat/protocols/twitter/twitter.js b/chat/protocols/twitter/twitter.js

>@@ -85,25 +86,60 @@ Tweet.prototype = {

>+    else if (this.outgoing && !this._deleted) {
>+      actions.push(
>+        new Action(_("action.delete"), function() {
>+          this.destroy(this._tweet);
>+        }, this)

The destroy method doesn't need the parameter (as it's part of the tweet, not of the conversation object), so you can simplify:
      actions.push(new Action(_("action.delete"), this.destroy, this));


>+  destroy: function(aTweet) {

The parameter is useless, its value is always equal to this._tweet.

>+    // Mark the tweet as deleted until we receive a response.
>+    this._deleted = true;
>+
>+    this.conversation._account.destroy(aTweet, this.onDestroyCallback,
>+                                       this.onDestroyErrorCallback, this);
>+  },
>+  onDestroyErrorCallback: function(aException, aData) {
>+    // The tweet was not successfully deleted.
>+    this._deleted = false;

delete this._deleted;

>+    this.conversation
>+        .systemMessage(_("error.delete", this.conversation._parseError(aData),
>+                         aTweet.text), true);

aTweet isn't defined here. Did you mean to use this.text?

The indent could probably look cleaner this way:
    let error = this.conversation._parseError(aData);
    this.conversation.systemMessage(_("error.delete", error, this.text), true);


>+  },
>+  onDestroyCallback: function(aData) {
>+    let tweet = JSON.parse(aData);
>+    // When unsuccessful an error thrown and the tweet is not marked as deleted.

This comment is hard to understand, please rephrase.

>+    if ("error" in tweet) {
>+      this._deleted = false;

delete this._deleted;

>+      throw tweet.error;

Is the point of the exception to make http.jsm call the error callback? If so, maybe make that explicit in the comment (+ in that case the error callback already takes care of deleting this._deleted)? If not, then who's catching it/displaying something interesting out of it?

>+    // When successful, the tweet is returned.
>+    let [text, name] = this.conversation.parseTweet(tweet);

Isn't the parsed text already available as this.text?

>@@ -178,17 +214,17 @@ Conversation.prototype = {
>         error = data.error;
>       else if ("errors" in data)
>         error = data.errors.split("\n")[0];
>       if (error)
>         error = "(" + error + ")";
>     } catch(e) {}
>     return error;
>   },
>-  displayTweet: function(aTweet) {
>+  parseTweet: function(aTweet) {

Even though I'm not sure the use case exposed in this patch is actually useful, I think it's a good idea to separate the parsing logic from the display logic.

>     let text = aTweet.text;
>     let entities = {};
>     // Handle retweets: retweeted_status contains the object for the original
>     // tweet that is being retweeted.
>     // If the retweet prefix ("RT @<username>: ") causes the tweet to be over
>     // 140 characters, ellipses will be added (and the truncated property is set
>     // to true). In this case, we want to get the FULL text from the original
>     // tweet and update the entities to match.
>@@ -291,16 +327,21 @@ Conversation.prototype = {
>           html += " title=\"" + entity.title + "\"";
>         html += ">" + ("text" in entity ? entity.text : entity.str) + "</a>";
>         text = text.slice(0, offset + entity.start) + html +
>                text.slice(offset + entity.end);
>         offset += html.length - (entity.end - entity.start);
>       }
>     }
> 
>+    return [text, name];

But the parseTweet method should only return the parsed text.

>+  },
>+  displayTweet: function(aTweet) {
>+    let [text, name] = this.parseTweet(aTweet);
>+

You can move here the only 2 lines of your parseTweet method that are related to the name.

>     let flags =
>       name == this._account.name ? {outgoing: true} : {incoming: true};
Attachment #8352727 - Flags: review?(florian) → review-
*** Original post on bio 683 at 2011-12-15 00:40:47 UTC ***

(In reply to comment #15)
> >+    this.conversation
> >+        .systemMessage(_("error.delete", this.conversation._parseError(aData),
> >+                         aTweet.text), true);
> 
> aTweet isn't defined here. Did you mean to use this.text?
Probably, but this.text seems to be undefined too.

> >+      throw tweet.error;
> Is the point of the exception to make http.jsm call the error callback? If so,
> maybe make that explicit in the comment (+ in that case the error callback
> already takes care of deleting this._deleted)? If not, then who's catching
> it/displaying something interesting out of it?
I made this into an ERROR call, should it actually be displayed in the UI (the timeline, I guess)?

> >+    // When successful, the tweet is returned.
> >+    let [text, name] = this.conversation.parseTweet(tweet);
> 
> Isn't the parsed text already available as this.text?
this.text seems to be undefined. prplIMessage doesn't define a text field (nor does jsProtoHelper or the Tweet object).

The rest of your comments I fixed. Should I store the output text into this.text (or this._text) and reuse it in the error/success methods?
*** Original post on bio 683 at 2011-12-15 11:14:28 UTC ***

(In reply to comment #16)

> > >+      throw tweet.error;
> > Is the point of the exception to make http.jsm call the error callback? If so,
> > maybe make that explicit in the comment (+ in that case the error callback
> > already takes care of deleting this._deleted)? If not, then who's catching
> > it/displaying something interesting out of it?
> I made this into an ERROR call, should it actually be displayed in the UI (the
> timeline, I guess)?

Have you considered the other solution my comment talked about? (throwing so that the error callback gets called) Was there a problem with it?

> 
> > >+    // When successful, the tweet is returned.
> > >+    let [text, name] = this.conversation.parseTweet(tweet);
> > 
> > Isn't the parsed text already available as this.text?
> this.text seems to be undefined. prplIMessage doesn't define a text field (nor
> does jsProtoHelper or the Tweet object).

My bad, I read the code too quickly and thought text was passed as a flag rather than a regular parameter of the tweet constructor.

The value should be available from this.originalMessage, which is defined in jsProtoHelper.
*** Original post on bio 683 at 2011-12-16 01:15:05 UTC ***

(In reply to comment #17)
> (In reply to comment #16)
> 
> > > >+      throw tweet.error;
> > > Is the point of the exception to make http.jsm call the error callback? If so,
> > > maybe make that explicit in the comment (+ in that case the error callback
> > > already takes care of deleting this._deleted)? If not, then who's catching
> > > it/displaying something interesting out of it?
> > I made this into an ERROR call, should it actually be displayed in the UI (the
> > timeline, I guess)?
> 
> Have you considered the other solution my comment talked about? (throwing so
> that the error callback gets called) Was there a problem with it?
Is this not what I originally had? I think originally http.jsm would get called, is this what you mean or is there a different "error callback" you mean?
Attached patch Delete action v5Splinter Review
*** Original post on bio 683 as attmnt 1113 at 2012-01-07 22:16:00 UTC ***

Changes:
 - This uses originalMessage when displaying the results of trying to delete.
 - Calls the error callback (by throwing) when an error is returned from twitter.com.
 - The parseTweet method now only returns the parsed text and not the name.
Attachment #8352855 - Flags: review?(florian)
Comment on attachment 8352727 [details] [diff] [review]
Delete action v4

*** Original change on bio 683 attmnt 986 at 2012-01-07 22:16:08 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352727 - Attachment is obsolete: true
Comment on attachment 8352855 [details] [diff] [review]
Delete action v5

*** Original change on bio 683 attmnt 1113 at 2012-01-15 16:18:54 UTC ***

Looks good. I haven't tested this version of the patch myself, but let's try this in nightlies :).
Attachment #8352855 - Flags: review?(florian) → review+
*** Original post on bio 683 at 2012-01-15 16:23:31 UTC ***

I'm editing the summary to reflect that the changes made here handle deleting tweets, not deleted tweets (we will need to do something with the delete notifications we receive on the user stream in another bug).
Summary: Handle deleted messages on twitter → Handle deleting messages on twitter
*** Original post on bio 683 at 2012-01-15 22:12:53 UTC ***

Checked in as http://hg.instantbird.org/instantbird/rev/1a4bc154d904
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.2
Blocks: 954669
You need to log in before you can comment on or make changes to this bug.