Changes introduced by the patch:

- A hidden flag for the pm_tags table (Hidden tags are not shown in the UI)
- A new "Inbox" tag, which is automatically created upon update/enable of the module.
- That tag is added for all recipients when a message is added.
- Instead of a huge HAVING, all we have to do now is add the Inbox tag to the filter
- Update function that adds Inbox tags to all message that are currently in the inbox
- An "Archive" Button, that allows to remove that Inbox tag

Please test but be aware that this hasn't been tested very well. It does only extend the schema/data so it *should* be save to remove the patch again.

This is imho required to fix #502664: remove duplicate records from pm_index and add primary key

CommentFileSizeAuthor
#23 privatemsg_inbox_tag10.patch16.75 KBberdir
#21 privatemsg_inbox_tag9.patch11.02 KBberdir
#18 privatemsg_inbox_tag8.patch9.98 KBberdir
#15 privatemsg_inbox_tag7.patch9.08 KBberdir
#12 privatemsg_inbox_tag6.patch9.6 KBberdir
#10 privatemsg_inbox_tag5.patch9.47 KBberdir
#8 privatemsg_inbox_tag4.patch9.17 KBberdir
#4 privatemsg_inbox_tag3.patch8.31 KBberdir
#3 privatemsg_inbox_tag2.patch9.75 KBberdir
privatemsg_inbox_tag.patch9.72 KBberdir

Comments

naheemsays’s picture

naheemsays commented

you AFAIK still won't be able to remive the author uid - just avoid duplicate data in that table as it would no longer be useful.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Correct :)

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
StatusFileSize
new privatemsg_inbox_tag2.patch9.75 KB

Re-roll of the patch to check qa.drupal.org integration and push the issue to the top :)

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
StatusFileSize
new privatemsg_inbox_tag3.patch8.31 KB

Automated test are not yet reported back, but they work!

The updated patch should pass all tests....

Status: Needs review » Needs work

The last submitted patch, privatemsg_inbox_tag3.patch, failed testing.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Needs review

#4: privatemsg_inbox_tag3.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, privatemsg_inbox_tag3.patch, failed testing.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Needs review
StatusFileSize
new privatemsg_inbox_tag4.patch9.17 KB

Trying to figure out what's going wrong with the test bot.

Status: Needs review » Needs work

The last submitted patch, privatemsg_inbox_tag4.patch, failed testing.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Needs review
StatusFileSize
new privatemsg_inbox_tag5.patch9.47 KB

Page not found error on edit tag url, strange. Adding more debug stuff.

Status: Needs review » Needs work

The last submitted patch, privatemsg_inbox_tag5.patch, failed testing.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Needs review
StatusFileSize
new privatemsg_inbox_tag6.patch9.6 KB

I think I got it. The test bot doesn't have clean url's enabled and drupal is inside a subdirectory.

While the new approach of that patch is still hacky but it should work. Let's see.

Will commit that fix as a separate change if it works.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

#12: privatemsg_inbox_tag6.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, privatemsg_inbox_tag6.patch, failed testing.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Needs review
StatusFileSize
new privatemsg_inbox_tag7.patch9.08 KB

Re-roll.

Status: Needs review » Needs work

The last submitted patch, privatemsg_inbox_tag7.patch, failed testing.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Needs review

#15: privatemsg_inbox_tag7.patch queued for re-testing.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
StatusFileSize
new privatemsg_inbox_tag8.patch9.98 KB

Improved the upgrade function, it does now go through all threads and processes them groupwise instead of looking for inbox messages over all users.

This is a huge scalabilty improvement, I've tested it with 2mio pm_index entries and it finished in a few minutes. The old version worked an hour without actually doing much.

I've also re-added the archive submit callback, which got lost in one of the earlier patches.

Warning: We probably need to do something about #717876: PM_Index ridiculous size. first, if we get more reports about that. the upgrade function will not convert the messages with thread_id 0. But, they won't show up correctly anyway right now.

naheemsays’s picture

naheemsays commented

Ok, just been playing with this:

1. There was no inbox tag (nor the variable "privatemsg_filter_inbox_tag") created for me when updating from privatemsg 1.1 - which leads me to assume the if statement in privatemsg_filter_update_6003 is wrong.

2. Moving on from 1, the new rows in pm_tags_index were created - with the tagid of 0. This actually made the functionality work (but then resaving the tagging data obviusly removed the 0 tag_id, breaking the finctionality again).

3. Enabling privatemsg_filter DOES create the new tag. However, the enable also needs to call privatemsg_filter_update_6004 in order to tag any previously sent/recieved messages.

4. I cannot get the archive button to show.

5. When tagging threads, the inbox tag is auto removed. There needs to be some check here to see if the tag previously exists and if so, to add it to the list of tags to be added.

naheemsays’s picture

naheemsays commented
Status: Needs review » Needs work
berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Needs review
StatusFileSize
new privatemsg_inbox_tag9.patch11.02 KB

Thanks for testing..

- Yeah, the broken update was my typical error : "if ($var = do_stuff() || do_other_stuff())" does *not* what you expect from it. It's actually "if ($var = (do_stuff() || do_other_stuff()))" instead of "if (($var = do_stuff()) || do_other_stuff())". This is fixed, updating should now work.

- The Archive button was missing because a necessary change in privatemsg.pages.inc was missing from the patch. Fixed and added a basic test for that feature.

naheemsays’s picture

naheemsays commented

This is fixed in privatemsg by in privatemsg_filter_remove_tags() replacing

  if (is_null($tag_id)) {
    // Delete all tag mapping.
    foreach ($threads as $thread) {
      db_query('DELETE FROM {pm_tags_index} WHERE uid = %d AND thread_id = %d', $account->uid, $thread);
    }
  }

with

  if (is_null($tag_id)) {
    // Delete all tag mapping - all except for the inbox tag if it exists.
    $inbox_tag = variable_get('privatemsg_filter_inbox_tag', '');
    foreach ($threads as $thread) {
      db_query('DELETE FROM {pm_tags_index} WHERE uid = %d AND thread_id = %d AND tag_id <> %d', $account->uid, $thread, $inbox_tag);
    }
  }

That just leaves number 3. Can we call privatemsg_filter_update_6004() privatemsg_filter_enable?

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
StatusFileSize
new privatemsg_inbox_tag10.patch16.75 KB

Ok, this is a first version with an admin page to rebuild the inbox by using batch api. seems to work fine for me...

Also displays a message informing users that enable the module to visit that page. And fixes the bug described above.

I'll add some tests for both the admin page and the fixed bug if I find some time...

crea’s picture

crea commented

Subscribing

YK85’s picture

YK85 commented

subscribing

naheemsays’s picture

naheemsays commented

This may be weird, but the tagging process seems to miss out a particular thread when adding the inbox tags.

Originally it had no replies, but since I have added one and it still is the case. I cannot see how it is different from all the other threads though.

And that one thread seems to be the only problem I can find.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

You tested the admin rebuild stuff and everything worked fine? Good.

Something that came to my mind is that we will need to adapt that once this and #502664: remove duplicate records from pm_index and add primary key because we will not be able to mark threads/messages that users sent to themself, but imho, we can live with that. The upgrade path will still work though.

What do you think, should we commit this so that we can continue with issues like #502664: remove duplicate records from pm_index and add primary key or wait until we've figured out what's up with that strange thread of yours? ;) Imho, it should be rather safe to go forward, we can't loose any data with this, atleast not without that other issue I've linked twice already :)

Oh, and regarding that thread, was that sent to another user or yourself, did you send it after/before the upgrade, .. ?

naheemsays’s picture

naheemsays commented

Before the upgrade. I think the second that should have been tagged. But i still think this should be comitted asap

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Fixed

Ok, I've commited this, let's see if we receive any bug reports about those threads or you can figure out something.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Version: » 7.x-1.x-dev
Status: Fixed » Patch (to be ported)

Almost forgot about 7.x :)

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Patch (to be ported) » Fixed

Commited the ported patch to D7 and also fixed dozens of broken tests due to changes in core.

Status: Fixed » Closed (fixed)

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