The following code is intended to grab records from ClosedOrders table and email to the user with a link to take a survey.
When the user receives the email, the use is supposed to get the following content:
This is a computer generated email message.
Please DO NOT use the REPLY button above to respond to this email.
Dear '+ @authorizedname +':
Thank you for using the order processing system.
Please click the link below to complete a survey
http://survey.php?wo=@ID
Regards,
Order administrator.'
So far, when the email comes through, the body is blank.
Only thing a user sees is the subject that says, "Feedback Survey"
Any ideas what I am doing wrong?
This has been an awesome forum.
Thank you very much.
ALTER proc [dbo].[GetUpdatedRecords]
@UserEmail nvarchar(200)
as
BEGIN
Declare @sender nvarchar(200)
declare @dept nvarchar(200)
declare @loc nvarchar(200)
declare @dteCreated nvarchar(20)
declare @ID nvarchar(12)
declare @email nvarchar(200)
declare @authorizedname nvarchar(200)
declare @message nvarchar(1000)
select @email = '', @message = 'SET QUOTED_IDENTIFIER OFF;
This is a computer generated email message.
Please DO NOT use the REPLY button above to respond to this email.
Dear '+ @authorizedname +':
Thank you for using the order processing system.
Please click the link below to complete a survey
http://survey.php?wo=@ID
Regards,
Order administrator.'
SELECT top 1 @email = @email+';'+Email, @message = @message
FROM ClosedOrder
WHERE status = 'Closed' and email=@UserEmail
EXEC msdb.dbo.sp_send_dbmail
@profile_name = 'Feedback', -- our defined email profile or static info
@recipients = @email,
@subject = 'Feedback Survey',
@body = @message;
Update ClosedOrder SET lsOpen = 1 WHERE lsOpen = 0 and email=@UserEmail
END
exec [GetUpdatedRecords] @userEmail='john.doe@yahoo.com'
Is that the entire code, or is there more to it. If it is the entire code, the @email will be null because the variables are not initialized, which means they will be null.
If you look through the code, @ID is declared, but it is not given a value. You have to give it a value (perhaps via a query?), or manually. For example, if @ID is always 123, you could do this:
DECLARE @dteCreated NVARCHAR(20)
DECLARE @ID NVARCHAR(12) = '123'; -- <---- GIVING ID A VALUE
DECLARE @email NVARCHAR(200) = ''
DECLARE @authorizedname NVARCHAR(200) = ''
DECLARE @message NVARCHAR(1000) = ''
SELECT @email = '' ,
@message = 'SET QUOTED_IDENTIFIER OFF;
This is a computer generated email message.
Please DO NOT use the REPLY button above to respond to this email.
Dear ' + @authorizedname + ':
Thank you for using the order processing system.
Please click the link below to complete a survey
http://survey.php?wo= ' + @ID + '
Regards,
Order administrator.'
Note that you have to concatenate the variable to the string for it to get the value.
The reason for @authorizedname being blank is the same. You have to find a way to give it a value.
Do not use a variable in a SELECT statement to concatenate values (that is, to compute aggregate values). Unexpected query results may occur. This is because all expressions in the SELECT list (including assignments) are not guaranteed to be executed exactly once for each output row. For more information, see this KB article.
I got the 123 passed to the link correctly but I need it as part of the query.
Adding it to the query below is not working:
SELECT top 1 @email = @email+';'+Email, @message = @message, @RequestID = RequestID,@authorizedname=InitiatedBy
FROM ClosedOrders
WHERE status = 'Closed' and email=@UserEmail
SET QUOTED_IDENTIFIER OFF;
select @message = '
This is a computer generated email message.
Please DO NOT use the REPLY button above to respond to this email.
Dear '+ @authorizedname +':
Thank you for using the order processing system.
Please click the link below to complete a survey
http://survey.php?wo= ' + @ID + '
Regards,
Order administrator.'
SELECT top 1 @email = @email+';'+Email, @message = @message, @ID = ID,@authorizedname=InitiatedBy
FROM ClosedOrders
Yes, it is because the query that fills in the values of the variables is after the statement where you compose the @message string. But, I did not understand how @ID could have gotten its value. In any case, can you try this?
ALTER PROC [dbo].[GetUpdatedRecords]
@UserEmail NVARCHAR(200)
AS
BEGIN
DECLARE @ID NVARCHAR(50) = ''
DECLARE @email NVARCHAR(200) = ''
DECLARE @authorizedname NVARCHAR(200) = ''
DECLARE @message NVARCHAR(1000) = ''
SET QUOTED_IDENTIFIER OFF;
SELECT TOP 1
@email = @email + ';' + Email ,
@message = @message ,
@ID = ID ,
@authorizedname = InitiatedBy
FROM ClosedOrders
WHERE status = 'Closed'
AND email = @UserEmail;
SELECT @message = '
This is a computer generated email message.
Please DO NOT use the REPLY button above to respond to this email.
Dear ' + @authorizedname + ':
Thank you for using the order processing system.
Please click the link below to complete a survey
http://survey.php?wo= ' + @ID + '
Regards,
Order administrator.'
EXEC msdb.dbo.sp_send_dbmail @profile_name = 'Feedback', -- our defined email profile or static info
@recipients = @email, @subject = 'Feedback Survey',
@body = @message;
UPDATE ClosedOrder
SET lsOpen = 1
WHERE lsOpen = 0
AND email = @UserEmail
END
GO
I don't know the default rules that the mail client might use for hyperlinking. One option might be to send the e-mail as html and explicitly specifying the hyperlink.
although setting it explicitly from the DB would ensure that it was capitalised as-stored in the DB (if that is important).
This
@message = @message ,
is doing nothing useful. Personally I would set the message here e.g.:
SELECT TOP 1
@email = Email ,
@message = '
This is a computer generated email message.
Please DO NOT use the REPLY button above to respond to this email.
Dear ' + InitiatedBy + ':
Thank you for using the order processing system.
Please click the link below to complete a survey
http://survey.php?wo= ' + ID + '
Regards,
Order administrator.'
FROM ClosedOrders
WHERE status = 'Closed'
AND email = @UserEmail;
What if [InitiatedBy] or [ID] are NULL? The whole message will be NULL ... presumably the message makes no sense (in this form) if either of those are NULL so just add
AND InitiatedBy IS NOT NULL
AND ID IS NOT NULL
to the WHERE clause. (Maybe those columns are defined as NOT NULL anyway? If this is an email going to end users, outside the company, I would leave them in anyway to prevent any future-accident as it would be embarrassing, to the company, to send a blank message! or a message with a LINK that does not work.
Also if ID is defined as a numeric datatype?? then it should be explicilty converted (in some circumsatnces SQL will implictly convert it, but I think it is better to always explicilty convert it then anyone else looking at the code, or you in a few years time!, will be able to see that the conversion was deliberate rather than accidental!)
If ID is numeric change
' + ID + '
to
' + CONVERT(varchar(20), ID) + '
There is no ORDER BY on the SELECT TOP 1. So it is entirely random which record is chosen (for a given email = @UserEmail) and thus which Survey ID is used. At the very least add an ORDER BY the Primary Key(s) for [ClosedOrders] so that the sequence is at least predictable / repeatable.
The Email is being sent based on
WHERE status = 'Closed'
but the update is being performed based on
WHERE lsOpen = 0
I would prefer to see them being preformed on the same criteria, but maybe this "difference" is deliberate / by design?
If nothing else the (lack of!) Normalisation would worry me that an email could be sent on an order that has status "Closed" and IsOpen = 1 or some variation of that.
In fact that will happen. The email is sent based on
WHERE status = 'Closed'
but that Status is then NOT changed, so the next time you run this the same user will get the email again, won't they?
I would prefer to see a Status that was a progression of numbers / stages:
10=Open
20=Closed but email not sent
30=Closed - email has been sent
so that the steps are clear and there aren't any "unanticipated" complications of multiple status codes that don't make sense!
If multiple such Status codes imply "Open" or "Closed" I would have a separate Status table:
StatusID - 10, 20, 30 as above
Name - i.e. "Open", "Closed but email not sent", "Closed after email sent"
IsOpen - either 0 or 1. Use this for reporting on all "Open" or "Not open" records
Thus there will be only ONE Status column in [ClosedOrders] but you can join to an [OrderStatus] table to see the Name and/or IsOpen state columns
I expect it is either a "don't care" or "too unlikely to happen", but there is an opportunity for a new ClosedOrder to be added between:
SELECT TOP 1
...
FROM ClosedOrders
WHERE status = 'Closed'
AND email = @UserEmail;
and
UPDATE ClosedOrder
SET lsOpen = 1
WHERE lsOpen = 0
AND email = @UserEmail
such that the user would not get an email for the later Closed Order. If that IS important I would do the update and generate the Message all-in-one statement like this:
BEGIN TRANSACTION
UPDATE ClosedOrder
SET lsOpen = 1,
@email = Email ,
@message = '
This is a computer generated email message.
Please DO NOT use the REPLY button above to respond to this email.
Dear ' + InitiatedBy + ':
Thank you for using the order processing system.
Please click the link below to complete a survey
http://survey.php?wo= ' + ID + '
Regards,
Order administrator.'
WHERE lsOpen = 0
AND email = @UserEmail
-- add this here? AND Status = 'Closed'
-- and this? AND InitiatedBy IS NOT NULL
-- and this? AND ID IS NOT NULL
-- Check for and abort if error detected here
EXEC msdb.dbo.sp_send_dbmail @profile_name = 'Feedback', -- our defined email profile or static info
@recipients = @email, @subject = 'Feedback Survey',
@body = @message;
-- Check for and abort if error detected here
COMMIT
First, you are absolutely right about the WHERE clause, where status='Closed'.
I should not have used it at all.
The initial reason to use @email = @email + ';' + Email was because we wanted code to have the ability to send email to several users at one.
However, since that idea has been scrapped, it means sense to just use @email=email.
About this:@message = @message
I actually tried your suggestion of using @message='This is a computer generated message...' but for some reason it didn't work. I will try it again shortly.
You asked:
What if [InitiatedBy] or [ID] are NULL?
This is how the table is setup.
An update is made in another table in another server.
We grab those updates where status='Closed' and dump them into ClosedOrders table.
Pretty much all fields in this table are required.
So, there is very, very little chance that InitiatedBy or ID is null but I will make your recommended change.
I thought that was probably the case. Obvious won't work if the EMail contains some data specific to the person and I guess that is where you are now
I would suggest that if you did want to send an email to multiple people you should use the BCC field, not the TO field, otherwise all recipients will see the email addresses of all others; that also leads to the risk that if one recipient has a virus etc. that could "harvest" the address of all the others from the TO (or CC) field in the Email.
That said I think there is benefit in sending the emails "singly". although its a pain to have to LOOP round generating each one, I think you will find it easier to debug any BOUNCE messages. Also if you want to change the link to
'http://survey.php?wo=' + ID + '&MySecretTrackingCode=' + MyCode
you can do that, which then enables you to see how many people clicked-through [that specific link], so even if the content of the email is not user-specific you might want some user-specific tracking codes in it. Well ... "we do" ... although I always think its a bit sneaky ...
What happens, say the myCode contains values with spaces, how do you handle that?
In my case, I handled it the following way, using REPLACE
'http://survey.php?wo=' + ID + '&MySecretTrackingCode=' + MyCode+'&loc='+Replace(@loc,' ', '%20')+'
It seems to work but not sure if there is a better way.
I would change spaces to "%20" as you have done. You might find that "+" also works if you prefer that (the URL will look "prettier")
Or if the space is there because the value is actually two separate pieces of information I would do
'&key1=' + Value1 + '&key2=' + Value 2
for us the MySecretTrackingCode is a (big!) integer, allocated in a database table ("LinksToEMailXReference") to be unique to the email, and also the location of the link within that email. So we are interested, for example, whether users only click links at the top of the email, or whether they also click links towards the bottom of the email / where they have to scroll.
If we want to know how many people clicks on the 3rd link we have to interrogate the LinksToEMailXReference table to find all the unique IDs used on emails for the 3rd link in that email run, and then how many of those were actually clicked on
I've lifted the link from a marketing email in my inbox (at random), here it is:
The link shows in my email here as "Click here to see COOL STUFF", the actual URL behind it has that tracking number "looking like an HTML file". I am sure that the "XLVsq3T98d5gyNDsYrbTORc" code is unique to me, AND unique to THAT url in THAT email ...
If you adopt that sort of practice then you never have a problem with spaces in your LINKS (but your EmailClickThroughTracking table might have quite a lot of rows in it!!!)
One of the values you are concatenating into the String must be NULL.
You can "fix" that using COALESCE() or ISNULL() like this:
SELECT TOP 1
@email = Email ,
@message = '
This is a computer generated email message.
Please DO NOT use the REPLY button above to respond to this email.
Dear '
+ COALESCE(InitiatedBy, '*** InitiatedBy IS NULL***')
+ ':
Thank you for using the order processing system.
Please click the link below to complete a survey
http://survey.php?wo= '
+ COALESCE(ID, '*** ID IS NULL***')
+ '
Regards,
Order administrator.'
FROM ClosedOrders
WHERE status = 'Closed'
AND email = @UserEmail;
if you want to check for both BLANK and NULL then use:
+ COALESCE(NullIf(RTrim(
COALESCE(InitiatedBy, '*** InitiatedBy IS NULL***')
), ''), '*** InitiatedBy IS BLANK ***')
although you don't really want the email going out with those messages in it, so for debugging only.
Better to prevent any emails going out like that by adding conditions to your WHERE clause to prevent anything going out with NULL / Blank values:
AND InitiatedBy IS NOT NULL
AND ID IS NOT NULL
AND RTrim(InitiatedBy) <> ''
AND RTrim(ID) <> ''
but the problem then is that IF that happens the Email does not go, but you do not get any sort of message to tell you that something needs fixing.
You can have a report:
WHERE
InitiatedBy IS NOT NULL
OR ID IS NOT NULL
OR RTrim(InitiatedBy) <> ''
OR RTrim(ID) <> ''
or you can first get all the data, for the email run, into a #TEMP table and then any "goofy rows" you could report on (send an email to yourself with the details), delete those from the #TEMP table and then EMail only the rows left in the #TEMP table (i.e. which are "clean")
