Support from Acquia helps fund testing for Drupal Acquia logo

Comments

doitDave’s picture

doitDave’s picture

Sorry, used the wrong patch format. Here's a unified patch file.

Berdir’s picture

Hm.

7.x-1.x is essentially in maintance mode as well, and your patch is renaming a function.

Can we do 7.x-2.x first and then decide if we want to backport (probably in a way that doesn't change the API)...

doitDave’s picture

Version: 7.x-1.x-dev » 7.x-2.x-dev
FileSize
5.11 KB

Here's the 7/2 patch, once one understands the internal settings structure, it is really nice to deal with. :)

OK, so I'll wait with doing the other branches until we are clear with this one. It's significantly less code here, I like that.

What do you think?

Berdir’s picture

Status: Patch (to be ported) » Needs review

Make sure to set patches to needs review, so that they are sent to the testbot.

+++ b/pm_email_notify/pm_email_notify.moduleundefined
@@ -97,6 +97,21 @@ function _pm_email_notify_only_user($uid) {
+ * Check if a user wants his email address as a sender.

Should be CheckS, according to the coding standards.

+++ b/pm_email_notify/pm_email_notify.moduleundefined
@@ -97,6 +97,21 @@ function _pm_email_notify_only_user($uid) {
+  // Either check the setting for this user or the global default.
+  $keys = array(
+    'user' => array($uid),
+    'global' => array(0),

You can also use privatemsg_get_default_setting_ids($account) for this. Will add id's for the role too but that doesn't matter much.

+++ b/pm_email_notify/pm_email_notify.moduleundefined
@@ -194,20 +209,26 @@ function pm_email_notify_send_mail($recipient, $message) {
+    if (privatemsg_get_setting('show_sender_email', array('user' => array($message->author->uid), 'global' => array(0)))) {

Not using your helper function here...

+++ b/pm_email_notify/pm_email_notify.moduleundefined
@@ -194,20 +209,26 @@ function pm_email_notify_send_mail($recipient, $message) {
+      // token replace for email from address

Can we fix that comment while we touch the code anyway? (start with upper case, end with .), also Token replacement sounds more like an english sentence to me.

Looks good otherwise.

doitDave’s picture

> Make sure to set patches to needs review, so that they are sent to the testbot.
My fault. Thx.

> Should be CheckS, according to the coding standards.
Honestly I just copied your helper function. Fixed it in both now.

> Will add id's for the role too but that doesn't matter much.
As far as I understand it, the role settings are only relevant for recipient settings, so would rather save the cost of a function call here. Or is that a faulty approach from your view?

> Not using your helper function here.
Fixed.

> Can we fix that comment while we touch the code anyway?
So I did. I also looked through all other comments, while we're at this. Same for some obviously outdated or missing doxygen infos. :)

Edit: Also catched a little typo of my own on the way.

Berdir’s picture

Looks good to me.

As far as I understand it, the role settings are only relevant for recipient settings, so would rather save the cost of a function call here. Or is that a faulty approach from your view?

I think it's less to type & maintain. For example, if we'd decide to rename 'global' to 'default', we would have to change less code if everything uses the helper function. And the cost of one function call isn't that high. The same argument could be made about your three line helper function that is used in two places ;) But I don't really care.

What I do care about are tests, though. It would be great to have basic test coverage for this, or we might break it at some point. Shouldn't be hard to add, have a look at the existing test coverage, and add some similar lines, with the only exception that you enable your setting and then make sure the sender address is correct.

doitDave’s picture

Puh! Got me. (Drupal's automatted) testing is something I really looked to escape from until now. Of course I am aware that can't hold that until ever. I would really have to start from scratch and I can't guarantee for anything...

So I wouldn't really be sad about knowing where to find the "existing test coverage", how to add something to it (also with patches? or what is the right way)? If you could mentor me a bit in that point, I would really be glad :)

Berdir’s picture

Can do that, but it's really not that hard. Just look at the pm_email_notify.test file, see also http://blog.worldempire.ch/story/writing-automated-tests-drupal-7, wich I've written a while ago. Or ping me in IRC if you have questions.

doitDave’s picture

OK, thanks! I think that's two good starting points. And if not, I will actually get back to you soon :)

doitDave’s picture

Thanks to your blog post and with a little code and API docs reading, it was really not as difficult as I first expected. I was new to simpletest but testing in general was not all new to me, maybe that helped. Setting up the test environment was actually the hardest part, as my hybrid dev environment (Windows and Linux mixed mode) was a bit non-conformistic, but I finally made that too. :)

I think (hope) I got everything right. I had to add a little non-invasive extension to your verifyMail() and verifyMails() methods and added one test with four topics of which I think they cover the new functionality. And as a benefit I can now finally extend my own modules with tests as well. Good thing!

Find attached the recent patch. Looking forward to your findings!

Berdir’s picture

+++ b/pm_email_notify/pm_email_notify.testundefined
@@ -352,7 +352,9 @@ class PrivatemsgEMailNotifyTestCase extends PrivatemsgBaseTestCase {
-    $this->assertNoText(t('Private messages'), t('Private messages fieldset is not displayed when there are no fields within it.'));
+    // This verification would never succeed. Recipient2 has read/write permissions
+    // and so he will always have the fieldset present to enable/disable pms.
+    // $this->assertNoText(t('Private messages'), t('Private messages fieldset is not displayed when there are no fields within it.'));

Hm. Enable/disable requires a special permission, is it possible that your addition is causing it to show up? Wondering if we want to add a permission for it, we do have quite a lot of permissions already, though.

+++ b/pm_email_notify/pm_email_notify.testundefined
@@ -367,6 +369,80 @@ class PrivatemsgEMailNotifyTestCase extends PrivatemsgBaseTestCase {
+    // Verify correct defaults.
+    $this->assertFieldByName('privatemsg_setting_show_sender_mail', 1, t("<em>Use sender's email address</em> value is now enabled in the form."));

The standards have changed on using t(), assertion messages (the third argument) should not use t() (button labels, the message you are looking for still should if the original does).

Also, only use an assertion message if it is actually useful, the one that is generated by default is often enough.

Test

doitDave’s picture

sure ...?

I just reinstalled the 7.x-2.x dev as it is in the repository and ran the test again. You are right, without the show_sender field, the alert wouldn't pop up. I forgot about the setting "allow disabling privatemessages". However, my setting is only shown to users with "write privatemessages" permission (because it only relates to authors), so IMO there is no logic flaw.

Indeed there are two options, either drop this test or add an additional permission. Honestly spoken I still vote for my solution (drop this particular assertion), as disallowing to hide their email address might be a privacy impact. However, we could also create recipient2 without "write" permission, as far as I see it, he never creates a message anyhow in the other tests, does he? That would be a way to keep the "field is empty check". So how do you decide?

The standards have changed on using t(), assertion messages (the third argument) should not use t() (button labels, the message you are looking for still should if the original does).

OK, I have removed the t(). Just for my knowledge, where would I look to know about the "official" current standards? The API page doesn't say anything on using t() (or not using it).

Also, only use an assertion message if it is actually useful

Isn't it if I want to make sure that the value is set correctly? What would you suggest instead?

Berdir’s picture

Removing the write permission for that user (or a new one) sounds good to me. The point there really is that we avoid showing a weird empty fieldset if no options are displayed, required some form API trickery...

Druplicon knows :)

assert t?
(09:33:52 AM) Druplicon: Do not use t() on assertion message texts. See http://drupal.org/simpletest-tutorial-drupal7#t and http://drupal.org/node/500866.

The assert there is fine to make sure that the value exists. But the custom assert message is not really necessary. All assert messages have default assertion messages, in this case it is "'Found field with name @name and value @value'", which is just as useful as yours I think.

doitDave’s picture

Removing the write permission for that user (or a new one) sounds good to me.

Done!

Druplicon knows :)

I've met that guy before ;)

OK, I think I also got the point regarding t(). You are right (I confess I did not have format_string() on my mind either).

Think this should be ready then? :)

(Just had to do some Worstpress coding today. It occurs to me ever and ever again that we are working on a *real* high level here regarding just everything. No kidding.)

Status: Needs review » Needs work

The last submitted patch, pm_email_notify-show_sender_mail-1880918-15.patch, failed testing.

doitDave’s picture

Oops. Wrong patch. Corrected one attached.

doitDave’s picture

Status: Needs work » Needs review
Berdir’s picture

Version: 7.x-2.x-dev » 6.x-2.x-dev
Status: Needs review » Patch (to be ported)

Yes, looks good.

Commited and pushed to 7.x-2.x. Next stop 6.x-2.x :)

doitDave’s picture

Status: Patch (to be ported) » Needs review
FileSize
15.16 KB

Here we go!

Berdir’s picture

Version: 6.x-2.x-dev » 7.x-1.x-dev
Status: Needs review » Patch (to be ported)

Looks great, commited!

Moving over to 7.x-1.x but as I said, if we want to add it there as well it needs to be in a way that is backwards compatible. Technically, not even adding new translatable strings is, but that can probably be overlooked :)

doitDave’s picture

Nice :)

And yeah, removing that function was quite stupid and near-sighted. Let's no longer talk of it ;) Also I have gotten a good part deeper into the module's internals, I think I will really drop the first attempt and do a proper backport instead. :)

To follow soon!

doitDave’s picture

Status: Patch (to be ported) » Needs review
FileSize
23.54 KB

So there it is.

A few additional annotations (I tried to be as verbose as possible in the comments).

  • Instead of removing _pm_email_notify_is_enabled() I turned it into a wrapper because I wanted the code to stay DRY'ish. From an external POV, the function has not changed so there should really be no possibility for breaking something. DB access and caching is now handled in the generic _pm_email_notify_setting() which might even be a wrapper itself just in case there will be a generic "settings API" in 1.x as well one day. However, if you prefer leaving the function's internals untouched, I can happily reset it.
  • I added default values to the settings' DB columns, to the new one as well to the existing one. Without that, I fear problems if a user e.g. lacks "write privatemsg" access and thus only saves half of the settings which would cause a DB error. I used the global initial defaults for both columns (that is "enable notification" and "don't show address". Regardless of what the site admin sets globally, this is (IMO) a safe fallback behaviour in any direction, what do you think?
  • As there was no dedicated test file for this submodule yet and I did not want to change the existing main module test, I also added a backport of the 2.x test file. In my local test runs, everything passed like it should, maybe you want to countercheck?

Looking forward to your thoughts!

doitDave’s picture

Status: Needs review » Patch (to be ported)
ptmkenny’s picture

Status: Patch (to be ported) » Needs review
doitDave’s picture

Issue summary: View changes

Let me know if something still happens, I am cleaning up & out.

ivnish’s picture

Status: Needs review » Closed (outdated)
andypost’s picture

Status: Closed (outdated) » Needs review

D7 is not yet outdated