The Message Private module provides a message type and associated entity reference fields to enable the sending and receiving of private messages using The Message Stack. Messages of type "Private Message" can be sent by creating and associating the message instance to user entities or OG group entities using the submodule.

See further information on the Message Private project page.

Clone the project: git clone --branch 7.x-1.x http://git.drupal.org/sandbox/mccrodp/2313725.git message_private

Module justification

Differences to Privatemsg module

  • Using Message Private on a site using The Message Stack removes the need for two separate message stacks on the one site
  • Message Private uses the more modern message stack and does not add any custom tables to the database which privatemsg does
  • Message Private provides full integration with views, Privatemsg does not provide full views integration on D7 #1573000: Preliminary Views integration
  • Privatemsg has no recent development (last commit 1 year ago) and is currently seeking co-maintainer
  • Privatemsg is currently more full featured than Message Private, but all features provided by Privatemsg are on Message Private development roadmap in the module README.md file

Reviews of other projects

Comments

PA robot’s picture

Status: Needs review » Needs work

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxmccrodp2313725git

I'm a robot and this is an automated message from Project Applications Scraper.

mccrodp’s picture

Status: Needs work » Needs review

Setting back to "Needs Review" - I updated the project to fix some of the code and comment 'errors' and 'warnings' not found by my offline version of CodeSniffer.

darol100’s picture

Manual Review

Individual user account
Yes Follows the guidelines for individual user accounts.
No duplication
They are very similar modules and he mention the differents on his project page.
Master Branch
Yes: Follows the guidelines for master branch.
Licensing
Yes Follows the licensing requirements
3rd party code
No
README.txt/README.md
Yes: Follows the guidelines for in-project documentation.
Code long/complex enough for review
Yes Follows the guidelines for project length and complexity.
Secure code
Yes

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This review uses the Project Application Review Template.

I ran the Pareview test and everything and doesnt have any errors. http://pareview.sh/pareview/httpgitdrupalorgsandboxmccrodp2313725git

pfu’s picture

Status: Needs review » Needs work

a.) In your README you just referenced Message, Message UI and Message Notify to be installed as a Requirement. There are a lot more needed e.g. Views, Chaos Tools, Entityreference, Features, Token. Please update the Requirements. I had to install ~9 other modules to install your module.
b.) I was a little confused how to send a new private message. When I go in the "Send" of my user account there's only "No messages created yet". The "Create a new message" is missing, only there if I got a message or sent a message. The link is also not correct, if I have installed drupal in a subdirectory your link is not working. *EDIT* Use drupal's url()
c.) I created a group 'tets' and I added a User 'test' if I send this message following error occures: "EntityFieldQueryException: Unknown field: og_user_node in EntityFieldQuery->addFieldCondition() (line 779 of /var/www/pfu/drupal/includes/entity.inc)." Something to do with the group, the error occures.
d.) Security is important, I don't know how you respond to spam yet. If a bot registers to drupal he can right away use the private message and spam all users. Maybe add some security phrase to it or something else.
e.) Documentation on usage would be good. Links where to go and how to change / add Message types. Also how to create new message etc. Also describe the permissions need to be set, so a new user can use the private message feature. I just used an admin account, but for a normal account it's kinda tricky.

mccrodp’s picture

Status: Needs work » Needs review

Hi, thanks for the reviews guys, especially @pfumi, your review was very helpful. I'll respond individually to your points.

  • a) You were right, I thought these modules were dependencies of the main modules I listed as "Requirements". Some are not, so need to be mentioned separately. I have cleaned up the "Requirements" and renamed it "Dependencies" as I am only listing direct dependencies. i.e. - not listing dependencies of dependencies. Both README and project page have been updated.
  • b) I have updated the views to display "Create new message" in the header and for an empty view results. I used the l() function, similar to your recommendation but seemed more suitable than url() in this case.
  • c) I could not reproduce your error, but I know how this error can come about using EntityFieldQuery. So, I changed method to get all group members using the OG API instead og_get_group_members_properties($group, array(), 'members', 'node');
  • d) I agree, security is important and needs to be mentioned. However, this module could be deployed in an intranet environment or other secure manner. Also, as is the same idea with Drupal core and other contrib, functional items should be de-coupled from each other. This module provides "Private Messages", and you can use another module for security. Therefore, I have updated the project page and the README to include a Security section and module recommendations.
  • e) I have added "How to use" section to the README and project page and will in future integrate with help or advanced_help modules.

Many thanks again for your informative review.

nettantra’s picture

Hi,

There are other modules which are using message stack for sending messages via a notifier plugin.
One is Message Notify which is a part of message stack that provides a framework for messaging and notifications.
How the Message Private module is different from Message Notify module.
Is there anything that this module provides which i can't accomplished using Message Notify module ?

Message Notify

mccrodp’s picture

Hi @nettantra. Message Notify is for notifying a user of a message's existence by sending the message via a plugin as you say. e.g. - notify a user that a new node was created via email. This is quite different to just sending private messages between users.

Message Notify provides an email and an SMS plugin. There is no "Private message" plugin for the ability to send custom messages between users and to groups. This message_private module actually uses message_notify and message_subscribe to send email notifications to users for private messages existing on the system for them. I hope to expose a new plugin to message_notify in future if possible for private messages within message_private. e.g. - Notify users that a new node was created on the system via private message.

You can see this issue on how the message stack does not provide this functionality at present #1671400: Using Message as private messaging service between users.

Message Private also bundles in an "Inbox" and "Sent" items view in the main module. No other modules I know of provide this using the Message Stack at present.

mccrodp’s picture

Issue summary: View changes
Issue tags: +PAReview: review bonus

Added Reviews of other projects section and issue tag PAReview: review bonus.

naveenvalecha’s picture

Assigned: Unassigned » naveenvalecha

Assigning to myself for next review.

mpdonadio’s picture

Assigned: naveenvalecha » Unassigned

@naveenvalecha, thanks for offering to review this project. Reviews by non-admins really makes our jobs easier. However, I am going to unassign you from this (but please post your review when you are done, and add it to your wiki page).

The review admins have been using Assigned for two things recently.

One, is to have a second admin do a review when the first marks a project as RTBC from Needs Review. We prefer two admin reviews where possible, and this helps speed the process up.

The other is that some admins assign themselves. I do this for a few reasons. One is to remind myself that I have something to do :) It also lets the applicant know as admin is looking at their project. It also helps save the project's spot in line, as the admins tend to work from the "oldest project needing response"; a further comment could push the application back down the queue. The assignment also tells the other admins that they can look at something else.

naveenvalecha’s picture

Thanks @mpdonadio for letting me know about that.I was assigning to myself for the next review so that may not another contributor was also going to contribute on the same.
I will post my review with my comments soon.

naveenvalecha’s picture

Status: Needs review » Needs work

https://www.drupal.org/node/2333455#comment-9173313
Hi @mccrodp,
Thanks for the contribution.

Automated Review

Best practice issues identified is none http://pareview.sh/pareview/httpgitdrupalorgsandboxmccrodp2313725git

Manual Review

Individual user account
Yes: Follows the guidelines for individual user accounts.
No duplication
May be: The differences are mentioned above in the issue summary, but regarding the duplication I am not sure.So feel good to hear about the this from git admins.Does not cause module duplication and fragmentation.
Master Branch
Yes: Follows the guidelines for master branch.
Licensing
Yes: Follows the licensing requirements
3rd party code
No
README.txt/README.md
Yes: Please fix those. Follows the guidelines for in-project documentation and the README Template.
Code long/complex enough for review
Yes/No: Follows the guidelines for project length and complexity.
Secure code
Yes. The module doesn't provide any custom form to get the user input.The user inputs are recieved the from the node add/edit/delete form.So the code is secured.
Coding style & Drupal API usage
  1. (*) you are overriding the access callback of the messsages pages(message/%message,message/%message/view,etc. ).Can't we just overide the access callback and also it will be very helpful if you specify the permissions of the bundles of all 'message types' enties.So that if any other created by the user itself s/he get this permission option available for that bundle too.Please reuse the code while doing this for different operations(view,edit,delete).What do you think ?
  2. (*)Adding this point to minor finding. Regarding the mail stuff in the hook_entity_insert function message_private_entity_insert($entity, $type) IMHO, we should write the rule provided by this module for this as we will get all the values in the bundle insert event.Then that will be manageable by site builder/module user to manage the rule easily.What do you think ?
  3. Specify the hook name in comments before the implemenation of hook_form_FORM_ID_alter. Before this function message_private_form_message_ui_instance_message_manage_alterin the
  4. There are typos in the Readme file.

This review uses the Project Application Review Template.

Thanks again

mccrodp’s picture

Hi @naveenvalecha,

Thanks for your review. I will work on some of the issues you raised in the coming days. I have some questions though so far on the points you raised.

  1. Yes, I agree, ideally I should not override the message callback provided by the message_ui module. I will try to somehow save my private messages to the path message-private/%message etc. However I'm slightly concerned that some of the message_ui functionality is based on the default path. If you have any recommendations or suggestions on this, I'd love to hear.
  2. Is this a major issue or just a design and functionality suggestion? I'm not sure I fully understand your comments here. You mean I should provide integration with the rules module here for allowing the user to fully configure notifications? This would be good, I will add it to my feature request list in the README for work on a separate branch. If this is infact a major issue, is providing an option to enable/disable email notifications acceptable for a stable release? i.e. - Is it major as you cannot currently disable email notifications per user? I have integration with flag module for enable/disable on my to-do list in the README.

I will add a more detailed list of reasons why this module is different to other existing private message modules such as Privatemsg to the project application issue next time I change the status. This seems to be unclear to reviews still. The main stand out difference is, that this message module unifies messages all under the same framework on your site. Any features that the message framework provides in the future can be easily integrated into this module also.

Using alternative message modules essentially is two different messaging stacks on the same site if you already require the message stack. Privatemsg has been around a lot longer and is certainly a more complete private messaging solution, but for simple private messaging using the message stack, this module serves it's purpose in my opinion and can only grow.

Thanks again for your review!

naveenvalecha’s picture

Hi @mccrodp,
Regarding #2, I have added it to minor finding and updated in my comment as well in the #12 so add this either to your feature list if you want.

Yes, I agree, ideally I should not override the message callback provided by the message_ui module. I will try to somehow save my private messages to the path message-private/%message etc. However I'm slightly concerned that some of the message_ui functionality is based on the default path. If you have any recommendations or suggestions on this, I'd love to hear.

Regarding this I would like if the message_ui module will provide the hook to just add the menu callback,access callback and page callback like here http://cgit.drupalcode.org/bundle_copy/tree/bundle_copy.api.php?id=refs/heads;id2=7.x-2.x This will also make the life easier of the other contrib projects that are using the message_ui module.Please add this to the feature request in the module.Now as it may take time so I would suggest you to override the access callback only.Please reuse the code while doing this for different operations(view,edit,delete).
Would you please add the differences of this module with others modules in the issue summary.
Thanks Again !

mccrodp’s picture

Hi @naveenvalecha, that's really a great help, much appreciated.

Your suggestion has given me an idea of how to do this until I can alter the access callback in a hook provided by message_ui in future. E.g.

$items['message/%message/view']['access callback'] = 'message_private_access_control';

function message_private_access_control($operation, $message, $user_obj = NULL) {
  // For all messages other than private_message use message_ui access control function.
  if($message->type != 'private_message') {
    return message_ui_access_control($operation, $message);
  } else {
    if($operation == 'edit' ) {
      // My custom access code for edit operation.
    }
    // Add other operations here.
  }
}

  1. I will add the feature request to message and link it to this issue
  2. Add the rules integration to my feature list roadmap
  3. Update the issue with the difference between this and other modules

Many thanks!

mccrodp’s picture

Issue summary: View changes
Status: Needs work » Needs review

Updates and corrections

  • Updated access checking and added Feature Request to Message UI as suggested #2345805: Provide hook to menu callback,access callback and page callback
  • Added views pre render hook to use attachment_before property instead of adding html to views header
  • Added flag for toggling on/off private_message email notifications on user account edit page
  • Change message to plain_text to remove dependency to filtered_html which may not exist
  • Email notification text can be customised using message_ui and with addition of flag, I am unsure what other essential features Rules can provide
  • Added Rules integration as topic to be investigated in README and changed extension to README.md
  • Merged all changes over to my dev branch for current working feature 'og_permissions' for greater OG permission integration
  • Added all dependencies to info file including dependencies of dependencies and added all contrib dependencies to README and project page
  • A new section "Similar modules" added to project page detailing Privatemsg module
  • Spell checked and could not see any typos in the README, just British English spelling of the word customisation
  • Re-wrote project application description and added clearer differences between available private message modules
klausi’s picture

Assigned: Unassigned » patrickd
Status: Needs review » Reviewed & tested by the community
Issue tags: -PAReview: review bonus

manual review:

  1. message_private.module: the module_load_include() calls in the beginning are not necessary, since that will load the files on every single page request. But the Features module only needs the field info when rebuilding the exported fields. You only the require_once on the *.features.in file.
  2. message_private_form_message_ui_instance_message_manage_alter(): doc block: which node edit form are you talking about?
  3. message_private_views_pre_render(): hm, why do we need that and cannot use the usual MENU_LOCAL_ACTION in hook_menu() that then shows up on the page? Please add a comment.
  4. message_private.views.inc: shouldn't the default view be in message_private.views_default.inc?
  5. I think you should remove the dependency on the Features module. People might want to customize the fields or whatever, so that would lead to an overridden feature or you need alter hooks, which is tedious. We had a similar situation in commerce_billy and then removed the Features dependency so that site-specific Feature modules can export the fields for example.
  6. message_private_og_entity_insert(): if you only want target message entities then you should use hook_message_insert() instead.
  7. message_private_access_control(): why do you need to perform user_load()s here? You are not accessing the user fields, so the objects present should be enough.
  8. message_private_access_control(): @return doc is useless. The datatype already tells me that this will return TRUE or FALSE. The comment should be something like "TRUE if the user is allowed to perform the operation on the message, FALSE otherwise.".
  9. message_private_access_control(): @param type for $message is wrong, this can also be a string.

But that are not critical application blockers, otherwise looks RTBC to me. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Assigning to patrickd as he might have time to take a final look at this.

mccrodp’s picture

Issue summary: View changes
Issue tags: +PAReview: review bonus
patrickd’s picture

sorry for the delay; would you mind to address or at least comment the issues klausi pointed out?

mccrodp’s picture

Thanks for the review @klausi. I should have commented on the issues I had addressed when I went through your comments initially. I didn't want to push my application further down the queue. I will work on the remainder of your issues in bold below on my og-permissions branch, along with completing the OG Permissions functionality over the coming days.

Work on og-permissions branch, (bolded items yet to be addressed):

  1. module_load_include() calls removed
  2. message_private_form_message_ui_instance_message_manage_alter(): doc block corrected
  3. "message_private_views_pre_render(): hm, why do we need that and cannot use the usual MENU_LOCAL_ACTION in hook_menu() that then shows up on the page? Please add a comment." - I don't fully understand this so far, but have a rough idea what you mean, so I will research further.
  4. Renamed views include: message_private.views_default.inc
  5. Removing Features dependency is high on my priority list for this module
  6. Will update to use hook_message_insert() next
  7. message_private_access_control(): @return doc updated
  8. message_private_access_control(): @param type updated

The default branch is still stable as far as I am aware. Thanks for all the reviews and comments.

patrickd’s picture

Status: Reviewed & tested by the community » Fixed

message_private.module #82
Please don't wrap lines like this as it might creates problem with the translation of these strings

    'description' => t('Grant to the user the ... option on
    any private messages. Grant this permission to trusted users!'),

Instead do this

    'description' => t('Grant to the user the ... option on' .
    'any private messages. Grant this permission to trusted users!'),

or just put it in one like

    'description' => t('Grant to the user the permission to apply CRUD option on any private messages. Grant this permission to trusted users!'),

See coding standards about line length:

Lines containing longer function names, function/class definitions, variable declarations, etc are allowed to exceed 80 chars.

-

Yes, you should definitely use MENU_LOCAL_ACTION instead of message_private_views_pre_render()

-

    'flag_short' => 'Activate Private Message Notifications',
    'flag_long' => 'Receive email notifications for new Private Messages',
    'flag_message' => 'Email notifications will be sent for new Private Messages',
    'unflag_short' => 'De-activate Private Message Notifications',
    'unflag_long' => 'Stop email notifications for new Private Messages',
    'unflag_message' => 'No notifications will be sent for new Private Messages ',

These should be passed through t() afaik

-

Thanks for your contribution!

I updated your account so you can promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and stay involved!

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.

mccrodp’s picture

Unfortunately I cannot use hook_message_insert as it is giving me an "Integrity constraint violation".

PDOException: SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '14' for key 'PRIMARY': INSERT INTO {message} (mid, type, arguments, uid, timestamp, language) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_placeholder_5); Array ( [:db_insert_placeholder_0] => 14 [:db_insert_placeholder_1] => private_message [:db_insert_placeholder_2] => a:0:{} [:db_insert_placeholder_3] => 1 [:db_insert_placeholder_4] => 1413687180 [:db_insert_placeholder_5] => en ) in drupal_write_record() (line 7207 of /Applications/MAMP/htdocs/local.message-private.test/includes/common.inc).

I believe this is due to the message_notify_send_message call, as you can see it also saves the message: https://www.drupal.org/node/2180227
Perhaps this tries to save the message twice and causes this violation. However I don't know why I do not get this error with hook_entity_insert. I could not find any documentation on hook_message_insert, is it some kind of hook_ENTITY_TYPE_insert? I will leave hook_entity_insert for now.

Also completed:

  • Fixed wrap lines at message_private.module #82
  • Wrapped flag description strings with translation function t()
  • Promoted project to full release and published reviewed master branch as alpha release
  • Work on the 'og_permissions' branch will be merged to the main branch when stable and release as dev release

I am still confused at the following.

...use MENU_LOCAL_ACTION instead of message_private_views_pre_render()

Does this mean I have to redefine the way my view is added, and add it via a page_callback? I tried instead on adding this via hook_menu_alter instead of hook_menu and excluding a page_callback, with no luck.

Do you have any reference articles or modules for this use case in adding a MENU_LOCAL_ACTION to an existing view or page? I can only find ones that create new pages using page callbacks. I'm also not sure about the access arguments v.s. page arguments yet, but the above issue is the main one. I'm sure I can figure out the rest once I know more about the approach you are recommending.

$items['user/%user/messages/inbox'] = array(
    'title' => 'Create a new private message',
    'description' => 'Select a message to create an instance.',
    'page callback' => /* Recommended approach? */
    'access callback' => 'message_private_access_control',
    'access arguments' => array('create', 'private-message'),
    'type' => MENU_LOCAL_ACTION,
    'weight' => -10,
  );

I am currently working on removing the Features dependency and providing proper integration with OG (permissions).

Many thanks to @patrickd for the final review and everyone else who reviewed this module.

Status: Fixed » Closed (fixed)

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