When users respond to an email and their mail client adds the "Re: " in front of the subject line - ogm should catch this and add it to the appropriate thread - sometimes it ends up creating a new thread.

Perhaps a check of the subject line using strpos with the original thread subject line and a separate strpos check for Re: could address this issue?

CommentFileSizeAuthor
#17 module_weight_fix.patch1.26 KBletapjar
#9 module_weight.patch1000 bytesletapjar
#8 module_weight.patch0 bytesletapjar

Comments

kyle_mathews’s picture

kyle_mathews commented
Status: Active » Postponed (maintainer needs more info)

Figuring out if a new email is meant as a comment to a previously created node or is a new node is the trickiest part of ogm and easily where I've spent the most time on this project.

So this isn't a very helpful bug report :) I'm quite aware that sometimes a new email isn't added correctly to an existing thread.

What you need to do is figure out how to reliably reproduce a bug. The problem is almost certainly an odd-ball email client that doesn't format their emails correctly. When you figure out how to reliably reproduce this bug, come back and write the exact conditions under which a new email isn't added correctly to its thread and attach the raw emails that the email client created. If you need help doing this please ask.

Also, if you want to hack on the algorithm for matching emails to threads, look at the og_mailinglist_parse_nid() function where ogm tries to extract a nid from the email.

letapjar’s picture

letapjar commented

OK will do - I was just putting it out there so the issue can be tracked. As I find the exact conditions I will report back.

letapjar’s picture

letapjar commented
Title: When users respond to a thread and the subject line changes to Re:[og-name] subject - OGM incorrectly creates a new thread » OGM should always send out emails AFTER pathauto has created it's automatic path alias

OK Looks like I mischaracterized this issue.

I have change the title here.

It appears that when a new thread is created from the website - OGM sends off the emails before pathauto has fired - this means that the footer says: "Full post: domain/node/nid" instead of
"Full post: domain/my-path-auto-alias"

This does not happen when new threads are created via email. Perhaps the difference is that ogm fires on hook_node when posts are created on the site and on hook_nodeapi when posts come in from email?

We should make it so that the correct path alias is always shown in the email footers - which can only happen by firing our hook after pathautho has done it's thing.

It does not appear that this creates 2 separate threads on the site as I orginally thought (that was some other issue with my own site config.)

kyle_mathews’s picture

kyle_mathews commented

If you can figure out how to do this, that'd be great :) It might just be a module weight issue, that OGM runs before Pathauto does. Try changing the weight of OGM in the system table to something higher than Pathauto to see if that makes a difference.

letapjar’s picture

letapjar commented

I will look at it - the other way would be to put the send mail part into hook nodeapi instead of hook node (I think) - I will report back once I try it.

kyle_mathews’s picture

kyle_mathews commented

Since hook_node doesn't exist :-) I doubt that can be the problem.

Posting on the web + through email do follow different pathways. Emails for posting on the web are triggered through hook_nodeapi and hook_comment. Posts coming through email are resent immediately so don't need a separate trigger.

letapjar’s picture

letapjar commented

looks like module weight fixes the issue. I manually changed the weight in my system table - but this shoulkd be part of the install process. I my drupal install pathauto had a weight of 1 while ogm and ogm forum had weights of 0.

letapjar’s picture

letapjar commented
Version: 6.x-1.x-dev » 6.x-1.0-beta3
Status: Postponed (maintainer needs more info) » Needs review
StatusFileSize
new module_weight.patch0 bytes

Here is a patch that adds 2 lines to hook)_install to set OGM's module weight to pathauto+1 to resolve the issue.

also, there was a query on line 319 that directly referenced 'node' table by name which breaks on installs where table prefixes are used - the patch also resolves this by adding the { } around the node table name.

letapjar’s picture

letapjar commented
StatusFileSize
new module_weight.patch1000 bytes

sorry - I accidentally uploaded an empty patch file in the last post (had already committed the change)

here is the patch

kyle_mathews’s picture

kyle_mathews commented

I've tested the the system table weight change fix on one of my sites and it works. Weight changes can sometimes have subtle implications so I'm going to sit on this change for a week or so before committing it to make sure nothing unexpected pops up.

On the patch, I went ahead and committed the unprefixed table name fix so you can remove that. You'll also need to write an update function so people already using the module can get the change. Also, if you could fix your comment to align it with the Drupal coding standards, that would be appreciated too--http://drupal.org/coding-standards#comment

letapjar’s picture

letapjar commented

by "write an update function" do you mean an update function for the module weight issue?

where would this go and what hook would fire it?

Do you want me to re-roll the patch w/o the table prefix part?

Sorry for the basic questions - I know how to program, but the shared coding thing is a new experience for me - I'm getting there though!

kyle_mathews’s picture

kyle_mathews commented

No problem :) I didn't know this stuff a few years ago either.

See this documentation: http://api.drupal.org/api/drupal/developer--hooks--install.php/function/...

Also, look at the end of og_mailinglist.install -- there's a number of update function I've written.

Whenever you make a database change in a module, you need to write an update function so that existing users of the module get the changes as well. Whenever you download an update to a module and run "updates", these update functions are what's running.

And yes, please re-roll the patch without the table prefix shift.

letapjar’s picture

letapjar commented

do I just choose the next number in line for the update function? (I just wrote a og_mailinglist_update_6005
function that sets the module weight. - is that the correct way?)

also - do you want me to remove the prefix only in the patch? I'm a bit confused on what to do for a patch in this thread vs. what to commit to my repo and submit a pull request for.

kyle_mathews’s picture

kyle_mathews commented

Yeah, that's the correct function name. Just put it right after og_mailinglist_update_6004().

Patches are simpler for small changes like this.

letapjar’s picture

letapjar commented

so, for the rerolled patch do you want the update function in there too?

kyle_mathews’s picture

kyle_mathews commented

Yes.

letapjar’s picture

letapjar commented
StatusFileSize
new module_weight_fix.patch1.26 KB

ok - I've re-rolled the patch - this now fixes the module weight issue only - and included the update function

(the table prefix issue is not fixed in this patch).

Kyle I now have 3 branches in my local repo how do you want me to push this to github - or will you pull the commit fom this patch?

kyle_mathews’s picture

kyle_mathews commented

A normal push (should) push all the branches up to github. That's actually my preferred (but somewhat more complex) way of collaborating via feature/bug fix branches as I can just add the branch you made the change with, fetch the changes, merge them into the main line, and commit. Which is simpler + let's me attribute to you the code changes.

kyle_mathews’s picture

kyle_mathews commented

Fixed with https://github.com/KyleAMathews/og_mailinglist/commit/c2e0c6d37ddef77a4f...

Thanks.

kyle_mathews’s picture

kyle_mathews commented
Status: Needs review » Fixed

And set to fixed now.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.