Closed (fixed)
Project:
Privatemsg
Component:
Code
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Reporter:
Created:
4 Aug 2008 at 23:53 UTC
Updated:
4 Sep 2008 at 03:47 UTC
Jump to comment: Most recent file
A place to track whatever crack i am trying. Not always good, but storing patches here means do not have to always care about making carefree changes/deletes on my working copy. And other potentially more experienced coders/hackers can tell me when I do stupid things (which I do a lot of).
What the current patch does:
1. Get rid of some of the dev stuff embedded into the module.
2. Theme the timestamp - I want to rename this variable too, but no idea how.
3. Changes to hook_user - only shoe message if the viewer has permission to write messages AND is not on own profile page (yeah I know you can send yourself messages... but no need for it on your own user page.).
4. Add a new message box to the bottom of the message page. I still need to work out how to pass the right details to it - I want to pass on thread_id, subject and recipients.
5. Incorporates patch from #288224: Show conversation instead of just single post.
6. Removed the words/references to facebook from most places (except one comment). Just because. yep.
Any pointers/insights would be welcome (especially to point 3).
| Comment | File | Size | Author |
|---|---|---|---|
| #9 | privatemsg_1.patch | 13.41 KB | litwol |
| #3 | privatemsg.patch | 13.43 KB | naheemsays |
| #2 | privatemsg.patch | 13.56 KB | naheemsays |
| #1 | working_copy_tabs.patch | 7.42 KB | naheemsays |
| working_copy.patch | 5.08 KB | naheemsays |
Comments
Comment #1
naheemsays commentedThe same as above, but the menu items as tabs instead of normal menu items. (Mark all as read does not work there... that needs to be elsewhere.)
Comment #2
naheemsays commentedNot complete, but I would like someone to review the stuff in this patch, just to know what to scrap and what to keep.
Additional changes:
1. I changed the tpl and css files, no where near complete and it looks crap (in some cases, worse the the prior facebookstyle), but they are more placeholder-type things where others can jump in too.
2. Added a working reply box on message view. This uses the existign privatemsg_new function and the additional code in there is extremely ugly as it has two loops to check for recipients and authors individually to populate the recipient box(ugh - any idea how to clean that up?). It also adds a thread_id (in privatemsg_new and the validate function)
Setting to needs review to get a review.
Comment #3
naheemsays commentedUpdated patch:
Less ugly code for getting the recipients. I will eventually want to hide these fields once everything is working well and reproducibly (which I think it is, but need to be sure.)
Still want a code review - some of this is probably crack, but other bits are an improvement, but hackish.
Can't help that as I less a developer, more a hacker.
Comment #4
mrtoner commentedHey, nbz! I've started to take a look at your issue and I'm going to break these out into separate issues to make it easier for the maintainer to review and commit. Here's what I've got so far:
#297835: Tabs, not tree
#297848: Overview not needed
#297994: Some minor wording changes
#298492: Various changes to message list
I'm also marking this against 6.x-1.x-dev, since it's tough to determine what HEAD means at any given time.Comment #5
naheemsays commentedThanks.
However, I think 6.x-1.x dev is massively out of date.
Comment #6
mrtoner commentedMmm, you're right. I didn't realize there was an actual -dev version that was older. The project sure needs updating! ("Under the new release system, even the HEAD releases of contributed projects are required to include version information and to indicate which versions of Drupal core they are compatible with.") I'm actually rolling patches against HEAD, not -dev.
Comment #7
naheemsays commented#299491: Put a reply form on the message page
Comment #8
litwol commentedStarting review...
Comment #9
litwol commented1) I've fixed a number of php notices due to some variables being used that are never defined or trying to access an undefined index of array.
2) Please change most if not all queries to use '_privatemsg_assemble_query', this function is very important that allows other modules to hook into how pmsg works. if we dont use it then we shut other modules out.
3) variable $thread on line 464 is not defined when i try to create a new message. lets make sure that this variable is handled in the scope that it exists.
4) when i try to send a new message and i click on 'preview' i get this error: notice: Undefined index: thread_id in /www/drupalgames.com/sites/all/modules/privatemsg/privatemsg.module on line 539. plus 2 errors from point (3).
5) inbox, sent messages and viewing message thread displays weird results. even though i've sent a message to many people, i should only see 1 message in that thread. right now i see my own message repeat many times. same result is in message thread, inbox and sent messages. lets fix the queries that pull those data.
6) I've added: When you send a message to a user from his profile, after sending the message you are redirected back to his profile.
7) Do we really need to insert 'Re: ' ? to me this looks very old fasioned. we can either take the subject box out of reply field or we can do something else with it. personaly i NEVER use the subject field after the message was first sent. not sure, maybe others like it then we should keep it.
8) Feature request: we should allow imagecache integration with the private message author pictures.
Attaching the new patch.
Comment #10
litwol commentedChanging status.
By the way, very good job guys. i see ALOT of progress. needless to say, without your help this module might have gone into nothingness. I will try to review patches more frequently now so you can have faster feedback and we can colaborate on this project together faster.
Comment #11
litwol commentedOn a second thought. is this patch a commulative of other patches ? or is this a patch of it's own? cross linking between patches is a bit confusing at 3:45 AM :-D.
can some one give a detailed breakdown which other patches are associated with this one and also rank which patches should go in first ? (sometimes people create patches which are on top of other patches that were made to HEAD, so the last patch wont apply without the first).
Comment #12
naheemsays commentedyes, this one is cumulative and the later comments (4 and 7) have split bits out into smaller chunks. No diea fi that is a good thing or not, but some of the smaller patches are (IMO) cleaner code, especially #299491: Put a reply form on the message page as instead of overloading privatemsg_new, I have put the changes into a privatemsg_form_alter function (and hidden the header as a whole.)
Thanks for the review - I am looking through it (and the other stuff you had a look at).
As for order, I would follow the order in comment 4 followed by comment 7 (which sort of relies on #288224: Show conversation instead of just single post as it sets the thread_id for a reply).
Comment #13
naheemsays commented3 and 4 should be fixed by #299491: Put a reply form on the message page
Other changes you have made to the patch have been incorporated into #298492: Various changes to message list, which should now be back to cnr with those changes.
Comment #14
mrtoner commented"7) Do we really need to insert 'Re: ' ? to me this looks very old fasioned. we can either take the subject box out of reply field or we can do something else with it. personaly i NEVER use the subject field after the message was first sent. not sure, maybe others like it then we should keep it."
+1 for removing the subject from the reply form.
Comment #15
litwol commentedwhats the status of this issue? a lot of things went into the module since this was created. maybe its best to close this one and start another ?
Comment #16
naheemsays commentedyes - most/all of the useful stuff has been separated out into its own patch/issue.