Problem/Motivation

Add a testing operation to digests that will allow for an admin to send a given digest to a given email / user. Useful for testing.

Proposed resolution

Add the feature. This should be on the list builder as an operation.

User interface changes

Adds an extra operation for admins.

API changes

Expands the scope of the digest, should not modify previous APIs or data models.

CommentFileSizeAuthor
#9 3175852-9.patch11.9 KBderekcresswell
#4 3175852-3.patch11.58 KBderekcresswell
#2 3175852-2.patch11.58 KBderekcresswell

Comments

DerekCresswell created an issue. See original summary.

derekcresswell’s picture

derekcresswell commented
Status: Active » Needs review
StatusFileSize
new 3175852-2.patch11.58 KB

Here is an implementation of this.

Two things I'd like discussed in the review :

  • Is "test" a good name? I didn't name it "Send now" because I thought that might be a seperate feature to send a digest to all users manually. What would be an alternate name for this operation?
  • Should this be included within 1.0.0? The security coverage might take a little while so it would probably be fine to fit it in.

Thanks.

Status: Needs review » Needs work

The last submitted patch, 2: 3175852-2.patch, failed testing. View results

derekcresswell’s picture

derekcresswell commented
Status: Needs work » Needs review
StatusFileSize
new 3175852-3.patch11.58 KB

Don't know how that didn't error before, missed one period.

gnumatrix’s picture

gnumatrix commented

Patch seems to work well.

I'd stick with 'Test', personally. I'd take 'Send Now' to mean something different as you suggest - sending out the digest to all the digest subscribers.

I read up a little tiny bit on the security coverage process. Certainly not encouraging in terms of whether I'd want to pursue Drupal development as a larger part of my day. So much ambiguity. If you can fit it in without interfering with that process, great! Wasn't clear to me whether that would be generally feasible or not. On the other hand, they can't possibly expect a module to sit in limbo indefinitely while waiting, so ?? Without understanding that fully, admittedly, I'd say it should be included.

derekcresswell’s picture

derekcresswell commented

Thank you for the input.

I might see about giving it another week or so but I don't think I'll continue to hesitate making a full release without security coverage.
While I don't like that I'll be adding new features after a RC is made, that's kinda my fault for making one so I will include this in the 1.0.0.

derekcresswell’s picture

derekcresswell commented
Parent issue: » #3166611: [Meta] Release version 1.0.0
derekcresswell’s picture

derekcresswell commented
Status: Needs review » Needs work

Going to mark this back as needs work.

Within #3178938: Permissions ignored on cron we are adding account switching within the send function of digests. This is to prevent permissions issues. I'd like to accommodate that here by changing this from send to any email to send to a user.

Sending this to a temporary user could prevent permissions from working correctly. The field on the form should be an autocomplete for users and default to the current account.

derekcresswell’s picture

derekcresswell commented
Status: Needs work » Needs review
StatusFileSize
new 3175852-9.patch11.9 KB

Now the form utilizes entity autocomplete instead of just an email.

gnumatrix’s picture

gnumatrix commented

Works very well.

travis-bradbury’s picture

travis-bradbury commented
Status: Needs review » Reviewed & tested by the community

Looks good to me too.

  • DerekCresswell committed f648166 on 1.0.x 
    Issue #3175852 by DerekCresswell, GNUMatrix: Add a send to user function...
derekcresswell’s picture

derekcresswell commented
Status: Reviewed & tested by the community » Fixed

Great. Committed.

derekcresswell’s picture

derekcresswell commented

Follow up, will make a new issue for this if wanted.

Should the test emails be demarcated as a test? I wouldn't want to change the body of the email (as that is what we want to show) but we could perhaps prefix the subject with 'TEST'. It wracks my nerves to see the digest in my live inbox and I go "but it looks so real, what if everyone got it?".

This would require the send function to be aware of the context in which it is being set. An extra array of parameters that we pass in for context. What I don't like about that is then the hook_mail and digest send are coupled to the context to sending, even if it's just as a pass-through parameter.

gnumatrix’s picture

gnumatrix commented

I don't know if it is worth the trouble? But if it is, maybe consider adding [TEST] as a subject suffix rather than prefix?

Status: Fixed » Closed (fixed)

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