Add internal threading to messages within an issue

Adds an "in_reply_to" field to Messages, which stores a reference to the Message the current message was in reply to. Defaults to the first Message in an issue if it exists, to handle e.g. inline comments where the new message is not internally a direct reply to another Message.

[Andi Albrecht, 2011-11-15 10:26:50]

[Corrected "techtonik" as reviewer :)]

IIRC there was a plan for a second change following this one? Am I right?

http://codereview.appspot.com/5370096/diff/1/codereview/models.py
File codereview/models.py (right):

http://codereview.appspot.com/5370096/diff/1/codereview/models.py#newcode85
codereview/models.py:85: first_msg = db.ReferenceProperty()  # reference to a
Message (defined below)
We could use Message as reference class in the consstructor of
ReferenceProperty(). This would make the comment obsolete too.

http://codereview.appspot.com/5370096/diff/1/codereview/models.py#newcode140
codereview/models.py:140: def first_message(self):
Missing docstring.

http://codereview.appspot.com/5370096/diff/1/codereview/models.py#newcode142
codereview/models.py:142: for msg in gql(Message,
Please use Message.all() here and construct the query with a Query object. We
try to use less GQL in favor of queries.

http://codereview.appspot.com/5370096/diff/1/codereview/models.py#newcode145
codereview/models.py:145: self.first_msg = msg
AFAICT issue.first_message is never saved. Or did I miss it somewhere in this
change?

http://codereview.appspot.com/5370096/diff/1/codereview/views.py
File codereview/views.py (right):

http://codereview.appspot.com/5370096/diff/1/codereview/views.py#newcode307
codereview/views.py:307: max_length=1000,
Indentation should align with "r" from "requried".

[techtonik, 2011-11-15 14:35:27]

On 2011/11/15 10:26:50, Andi Albrecht wrote:
> [Corrected "techtonik" as reviewer :)]
> 
> IIRC there was a plan for a second change following this one? Am I right?

Yes. The plan is to fix threading in emails, but to make code more maintainable,
we should implement generic threading for messages first. (Thanks for
correction)

[Kaelyn, 2011-11-21 22:05:46]

Yes there is a second patch that adds the email threading; I'm still working on
it and wanted to get this review started while doing so. ;)

http://codereview.appspot.com/5370096/diff/1/codereview/models.py
File codereview/models.py (right):

http://codereview.appspot.com/5370096/diff/1/codereview/models.py#newcode85
codereview/models.py:85: first_msg = db.ReferenceProperty()  # reference to a
Message (defined below)
On 2011/11/15 10:26:51, Andi Albrecht wrote:
> We could use Message as reference class in the consstructor of
> ReferenceProperty(). This would make the comment obsolete too.

I tried that but it doesn't work since the Message class is not yet defined (the
definition is about 110 lines down), and as far as I know there is no Python
equivalent to a C/C++ forward declaration. Also, the definition of the Message
class cannot be moved to before the Issue class definition since it already uses
Issue as a reference class for a db.ReferenceProperty.

http://codereview.appspot.com/5370096/diff/1/codereview/models.py#newcode140
codereview/models.py:140: def first_message(self):
On 2011/11/15 10:26:51, Andi Albrecht wrote:
> Missing docstring.

Added.

http://codereview.appspot.com/5370096/diff/1/codereview/models.py#newcode142
codereview/models.py:142: for msg in gql(Message,
On 2011/11/15 10:26:51, Andi Albrecht wrote:
> Please use Message.all() here and construct the query with a Query object. We
> try to use less GQL in favor of queries.

FYI, in the code under codereview/, GQL seems to be favored over .all() by a
factor of almost 3:1 (27 occurrences of ".gql" to 10 of ".all"). GQL is also
exclusively used in Issue's methods, including num_comments and
_get_num_comments which first_message was based on.

After a little testing to make sure the Query object returns the same results as
the gql when using .filter('in_reply_to =', None)--as the docs weren't clear
about the filter() equivalent for "property = NULL"--I've replaced the gql
query.

http://codereview.appspot.com/5370096/diff/1/codereview/models.py#newcode145
codereview/models.py:145: self.first_msg = msg
On 2011/11/15 10:26:51, Andi Albrecht wrote:
> AFAICT issue.first_message is never saved. Or did I miss it somewhere in this
> change?

The code was modeled after issue.n_comments/issue.num_comments, which also does
not appear to be explicitly saved (certainly not within the definition of
Issue). It is mainly intended as an internal cache so that the query for looking
up issue.first_message only needs to be run once per issue in a single server
instance. It does appear to be saved at some point, since when I restarted my
dev instance, the code for when issue.first_msg is not true was not executed
when issue.first_message was accessed.

http://codereview.appspot.com/5370096/diff/1/codereview/views.py
File codereview/views.py (right):

http://codereview.appspot.com/5370096/diff/1/codereview/views.py#newcode307
codereview/views.py:307: max_length=1000,
On 2011/11/15 10:26:51, Andi Albrecht wrote:
> Indentation should align with "r" from "requried". 

Whoops. Fixed.

[Andi Albrecht, 2011-11-22 07:41:22]

http://codereview.appspot.com/5370096/diff/1/codereview/models.py
File codereview/models.py (right):

http://codereview.appspot.com/5370096/diff/1/codereview/models.py#newcode85
codereview/models.py:85: first_msg = db.ReferenceProperty()  # reference to a
Message (defined below)
On 2011/11/21 22:05:46, Kaelyn wrote:
> On 2011/11/15 10:26:51, Andi Albrecht wrote:
> > We could use Message as reference class in the consstructor of
> > ReferenceProperty(). This would make the comment obsolete too.
> 
> I tried that but it doesn't work since the Message class is not yet defined
(the
> definition is about 110 lines down), and as far as I know there is no Python
> equivalent to a C/C++ forward declaration. Also, the definition of the Message
> class cannot be moved to before the Issue class definition since it already
uses
> Issue as a reference class for a db.ReferenceProperty.

ah, right. Sorry, I've missed that and thought that it allows string references
like Django, i.e. ReferenceProperty('Message'), but that isn't the case.

[techtonik, 2011-11-22 09:50:07]

I think the current approach with making Issue knowing about it's messages is no
good.

http://codereview.appspot.com/5370096/diff/1/codereview/models.py
File codereview/models.py (right):

http://codereview.appspot.com/5370096/diff/1/codereview/models.py#newcode85
codereview/models.py:85: first_msg = db.ReferenceProperty()  # reference to a
Message (defined below)
IMO it is good we've stumbled upon this. How about decoupling issue from message
threads completely, moving all relevant logic into Message model?

This will require only single function:

Message.get_thread(Issue or issue_id) to return all messages for the given issue
_ordered by time_ (view can additionally sort them by nesting level if desired).

http://codereview.appspot.com/5370096/diff/1/codereview/views.py
File codereview/views.py (right):

http://codereview.appspot.com/5370096/diff/1/codereview/views.py#newcode3110
codereview/views.py:3110: in_reply_to = issue.first_message
For web we should not link all replies to the first message. It is completely ok
if somebody 'creates new top thread on the issue'.

We need thread id only for emails, so let's try to create issue specific email
thread with separate patch.

http://codereview.appspot.com/5370096/diff/1/static/script.js
File static/script.js (right):

http://codereview.appspot.com/5370096/diff/1/static/script.js#newcode659
static/script.js:659: form.in_reply_to.value = message_key;
I am confused. message_id is not Message.id, but HTML div id. Can we replace key
with true message id?

[Kaelyn, 2011-11-22 18:26:37]

For the record, Issue.first_msg and Issue.first_message were about caching a
reference to a *single* message within the issue, for use as a default
in_reply_to when creating new Messages. Issue.first_message never fetched more
than one message from the datastore, much less all of the messages associated
with the issue.

http://codereview.appspot.com/5370096/diff/1/codereview/models.py
File codereview/models.py (right):

http://codereview.appspot.com/5370096/diff/1/codereview/models.py#newcode85
codereview/models.py:85: first_msg = db.ReferenceProperty()  # reference to a
Message (defined below)
On 2011/11/22 09:50:07, techtonik wrote:
> IMO it is good we've stumbled upon this. How about decoupling issue from
message
> threads completely, moving all relevant logic into Message model?

Given the logic is about finding a single Message in an issue that is not a
reply to another Message, moving it into the Message model would be pointless.
All messages in an issue would return the same message, but the change would
require a query *every* time a new message is created without explicitly being
in reply to another message. Not to mention there would be no reason to cache
the value since it would only be used when the given message is being created.
At that point, one might as well just do the query when necessary within
_make_message.

> 
> This will require only single function:
> 
> Message.get_thread(Issue or issue_id) to return all messages for the given
issue
> _ordered by time_ (view can additionally sort them by nesting level if
desired).

That function is useless as the only reason for first_msg is to provide a
default message to use as an in_reply_to. It is *not* about accessing all of the
messages for an issue or even for an entire thread (note, for example, that
there is no link from a message to the next message(s) in a thread).

Anyway, all of the above is mute as I've removed the issue.first_message logic
and usages from this patch.

http://codereview.appspot.com/5370096/diff/1/codereview/views.py
File codereview/views.py (right):

http://codereview.appspot.com/5370096/diff/1/codereview/views.py#newcode3110
codereview/views.py:3110: in_reply_to = issue.first_message
On 2011/11/22 09:50:07, techtonik wrote:
> For web we should not link all replies to the first message. It is completely
ok
> if somebody 'creates new top thread on the issue'.
> 
> We need thread id only for emails, so let's try to create issue specific email
> thread with separate patch.

Fair enough. My intent in linking replies to the first message if in_reply_to
wasn't already passed in to _make_message was to handle the cases--such as
replies to inline comments made in the code views--that aren't explicit replies
to a given Message. I've removed the issue.first_message stuff and will leave
the trickier threading cases to whoever implements the threaded message view on
the web pages.

http://codereview.appspot.com/5370096/diff/1/static/script.js
File static/script.js (right):

http://codereview.appspot.com/5370096/diff/1/static/script.js#newcode659
static/script.js:659: form.in_reply_to.value = message_key;
On 2011/11/22 09:50:07, techtonik wrote:
> I am confused. message_id is not Message.id, but HTML div id. Can we replace
key
> with true message id?

I originally changed the HTML to use the Message object key as message_id and
got this function to work properly, but it broke other, shared JavaScript
functions and trying to fix them all without breaking any of the web pages
required far more modifications to the JS than appropriate for this patch and
far more web testing than I am willing or have the time to do. For example, the
value in the HTML form passed to this function as the message_id is passed to
other functions and IIRC in at least one case hits code that assumes the value
is an integer. I've renamed message_key to make it clearer that it is the
(string) value of the db.Model.key() inherited by the Message model class. (FYI,
there is no Message.id)

[techtonik, 2011-11-23 16:02:40]

With the except of small nit below, seems good to me. Andi?

http://codereview.appspot.com/5370096/diff/11002/codereview/views.py
File codereview/views.py (right):

http://codereview.appspot.com/5370096/diff/11002/codereview/views.py#newcode3134
codereview/views.py:3134: logging.warn('Invalid in-reply-to Message or key
given: %s', in_reply_to)
We also need to compare issues to catch DB inconsistencies earlier.

[Kaelyn, 2011-11-23 17:41:15]

http://codereview.appspot.com/5370096/diff/11002/codereview/views.py
File codereview/views.py (right):

http://codereview.appspot.com/5370096/diff/11002/codereview/views.py#newcode3134
codereview/views.py:3134: logging.warn('Invalid in-reply-to Message or key
given: %s', in_reply_to)
On 2011/11/23 16:02:40, techtonik wrote:
> We also need to compare issues to catch DB inconsistencies earlier.

Done.

[Andi Albrecht, 2011-11-23 18:59:06]

Two other nits. But looks good then :)

http://codereview.appspot.com/5370096/diff/14003/codereview/views.py
File codereview/views.py (right):

http://codereview.appspot.com/5370096/diff/14003/codereview/views.py#newcode307
codereview/views.py:307: max_length=1000,
Have you seen the latest change to the tip? We now have module constants for
max_length in forms. It would be nice if you could update your patch to the
latest source once again to use these constants and to make sure that it still
applies cleanly.

(Sorry for this last minute change :)

http://codereview.appspot.com/5370096/diff/14003/codereview/views.py#newcode3139
codereview/views.py:3139: except (db.KindError, db.BadKeyError):
Which of these statements can raise db.KindError? And if I'm not wrong, only
models.Message.get(in_reply_to) could raise db.BadKeyEror. So the try/except
block should only guard this line then.

[Kaelyn, 2011-11-23 21:20:51]

http://codereview.appspot.com/5370096/diff/14003/codereview/views.py
File codereview/views.py (right):

http://codereview.appspot.com/5370096/diff/14003/codereview/views.py#newcode307
codereview/views.py:307: max_length=1000,
On 2011/11/23 18:59:06, Andi Albrecht wrote:
> Have you seen the latest change to the tip? We now have module constants for
> max_length in forms. It would be nice if you could update your patch to the
> latest source once again to use these constants and to make sure that it still
> applies cleanly.
> 
> (Sorry for this last minute change :)

No, I hadn't seen the latest changes to the tip. Changed the max_length to be a
module constant (and the changes already applied cleanly ^.^).

http://codereview.appspot.com/5370096/diff/14003/codereview/views.py#newcode3139
codereview/views.py:3139: except (db.KindError, db.BadKeyError):
On 2011/11/23 18:59:06, Andi Albrecht wrote:
> Which of these statements can raise db.KindError? And if I'm not wrong, only
> models.Message.get(in_reply_to) could raise db.BadKeyEror. So the try/except
> block should only guard this line then.

models.Message.get will raise db.KindError if the looked-up object is not an
instance of models.Message--such as if in_reply_to was passed the key string for
an Issue or a Patch.

The try/except is around the whole block because the other lines (the check to
make sure the in_reply_to Message is a message in the current issue) should
not--and don't need to be--executed if the models.Message.get fails. As the
Message.get will never return None, the code is clearer and cleaner by having
that extra checking code within the try/except instead of immediately afterward
under an "if msg.in_reply_to:" (which, as I mentioned, would only not be true if
the Message.get triggered an exception).

[Andi Albrecht, 2011-11-24 11:25:11]

Looks good to me. Anatoly, do you have any comments. You're more familiar with
this change than I.

[Kaelyn, 2011-11-29 00:00:35]

On a side note, I have a patch for the email threading mostly done but am
lacking a reliable way to associate an email Message-ID with an existing
Message instance (right now I've been testing the patch by CCing a special
email address that includes the string db.Key of the Message instance).
I've filed http://code.google.com/p/googleappengine/issues/detail?id=6426
asking
for either an API call to obtain the email Message-ID of a sent email, or
to allow custom X-foo (or X-Google-Appengine-foo or similar) email headers
to be set.

Cheers,
Kaelyn

[gvrpython, 2011-11-29 00:21:59]

Unfortunately I don't think that feature request will be satisfied any time
soon. Obtaining the message-id after the message was sent is the least
likely; setting the Message-id is probably a security problem. I'll ask the
proper authorities though.

On Mon, Nov 28, 2011 at 4:00 PM, Kaelyn Uhrain <rikka@google.com> wrote:

> On a side note, I have a patch for the email threading mostly done but am
> lacking a reliable way to associate an email Message-ID with an existing
> Message instance (right now I've been testing the patch by CCing a special
> email address that includes the string db.Key of the Message instance).
> I've filed http://code.google.com/p/googleappengine/issues/detail?id=6426
asking
> for either an API call to obtain the email Message-ID of a sent email, or
> to allow custom X-foo (or X-Google-Appengine-foo or similar) email headers
> to be set.
>
> Cheers,
> Kaelyn
>
> --
> You received this message because you are subscribed to the Google Groups
> "codereview-discuss" group.
> To post to this group, send email to codereview-discuss@googlegroups.com.
> To unsubscribe from this group, send email to
> codereview-discuss+unsubscribe@googlegroups.com.
> For more options, visit this group at
> http://groups.google.com/group/codereview-discuss?hl=en.
>



-- 
--Guido van Rossum (python.org/~guido)

[techtonik, 2011-11-29 17:05:32]

LGTM and committed to
https://code.google.com/p/rietveld/source/detail?r=a140eab7636e77e29d562d651c...

Thanks Kaelyn now it's possible to implement threaded conversations in web
interface. The funny thing is that I pushed commit using your name. =)

[techtonik, 2011-11-29 17:10:30]

On 2011/11/29 00:21:59, gvrpython wrote:
> Unfortunately I don't think that feature request will be satisfied any time
> soon. Obtaining the message-id after the message was sent is the least
> likely; 

Although I'd prefer to move this discussion to a separate thread, I'll ask
anyway. Why it is not possible to obtain message-id? It is only logical if
send_mail() at
http://code.google.com/appengine/docs/python/mail/functions.html#send_mail
returned message-id that could later be used to query message delivery status
other stuff.