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)