Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
As discussed via mail, this is the 7.x-1.x FR for integrating http://drupal.org/project/privatemsg_notify_sender into pm_email_notify. I will add FRs for the other branches as follow-ups. Patch will be in the first comment.
Comments
Comment #1
doitDave CreditAttribution: doitDave commentedAttached the patches for this branch.
Comment #2
doitDave CreditAttribution: doitDave commentedSorry, used the wrong patch format. Here's a unified patch file.
Comment #3
BerdirHm.
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)...
Comment #4
doitDave CreditAttribution: doitDave commentedHere'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?
Comment #5
BerdirMake sure to set patches to needs review, so that they are sent to the testbot.
Should be CheckS, according to the coding standards.
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.
Not using your helper function here...
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.
Comment #6
doitDave CreditAttribution: doitDave commented> 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.
Comment #7
BerdirLooks good to me.
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.
Comment #8
doitDave CreditAttribution: doitDave commentedPuh! 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 :)
Comment #9
BerdirCan 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.
Comment #10
doitDave CreditAttribution: doitDave commentedOK, thanks! I think that's two good starting points. And if not, I will actually get back to you soon :)
Comment #11
doitDave CreditAttribution: doitDave commentedThanks 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!
Comment #12
BerdirHm. 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.
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
Comment #13
doitDave CreditAttribution: doitDave commentedI 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?
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).
Isn't it if I want to make sure that the value is set correctly? What would you suggest instead?
Comment #14
BerdirRemoving 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.
Comment #15
doitDave CreditAttribution: doitDave commentedDone!
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.)
Comment #17
doitDave CreditAttribution: doitDave commentedOops. Wrong patch. Corrected one attached.
Comment #18
doitDave CreditAttribution: doitDave commentedComment #19
BerdirYes, looks good.
Commited and pushed to 7.x-2.x. Next stop 6.x-2.x :)
Comment #20
doitDave CreditAttribution: doitDave commentedHere we go!
Comment #21
BerdirLooks 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 :)
Comment #22
doitDave CreditAttribution: doitDave commentedNice :)
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!
Comment #23
doitDave CreditAttribution: doitDave commentedSo there it is.
A few additional annotations (I tried to be as verbose as possible in the comments).
Looking forward to your thoughts!
Comment #24
doitDave CreditAttribution: doitDave commentedComment #25
ptmkenny CreditAttribution: ptmkenny commentedComment #26
doitDave CreditAttribution: doitDave commentedLet me know if something still happens, I am cleaning up & out.
Comment #27
ivnish CreditAttribution: ivnish commentedComment #28
andypostD7 is not yet outdated