Hey Jason,
Thanks for the work on this. A few thoughts.
- In uc_gift_certificate_create_new_certs() we're looking for either a msg_to_recipient or email_address attribute, but this isn't flexible enough to allow us to create printed certificates. I would suggest that instead of this, we should just look for the "Dollar Amount" attribute first (and then we can check to see if it's an emailed certificate or not). Or, alternatively, we can use that as a third elseif, as I've done here:
// Check if the product is a gift certificate
if ($data && $data['attributes']) {
// Attribute is stored using label
foreach ($data['attributes'] as $label => $vals) {
$attrib = _uc_gc_find_attrib_by_label($label, $product->nid);
if ($attrib->aid == $email_attrib->aid) {
$is_gift_cert = TRUE;
$recipient_email = $vals[0];
}
elseif ($attrib->aid == $msg_attrib->aid) {
$is_gift_cert = TRUE;
$msg_to_recipient = $vals[0];
}
elseif ($label == 'Dollar Amount') {
$is_gift_cert = TRUE;
// Use the purchaser's email?
}
}
- Since we won't have an email address to notify the person (recipient) with, perhaps we should attach the purchaser's email address, and send them an email with the code and a notification that a certificate was generated and will be shipped to them. This could be optional, of course.
Anyway, I feel like Dollar Amount will always be a part of a certificate, printed or electronic, so we should probably include that in the logic somewhere.
- You'll also notice I added $is_gift_cert = TRUE to the 2nd elseif. This was missing but I'm not sure why; if the product has a msg to recipient, that probably means it's a certificate, but I'm not sure this will always be the case. I am wondering if not having it there would cause some certificate purchases to fail.
I had to make these changes to my working version of the module to accommodate for printed certificates, which are always huge around the holidays. I'd like to rethink some of this but without negating the attribute-checking logic you've written. I think the logic you've included is good, to determine whether or not it needs to use the $vars[0] as an email address or a message, but we can improve it a little bit for the use-cases where a recipient might be receiving a gift card in lieu of an email.
Comments
Comment #1
jrust commentedWhere are you getting Dollar amount attrib? That doesn't seem to be a standard Gift cert attribute. Our store just uses the price field or UC_varprice module for variable price certs
Comment #2
torgospizzaOh, hmm. Perhaps you're right. In our store, we have a drop down for multiple price points. (uc_varprice is a great idea, though I have no experience with it... does it allow for "write-in" textfield attributes?)
So maybe I'll change this. I'm thinking then, that maybe "Dollar Amount" should be a standard attribute as well, that gets added. I had thought it was added programmatically when the module is enabled, but I guess not. The price field is a good place to start, but we like giving people one product with many options, like starting at $5 (the base price) and the dropdown goes up to $100. See: www.rifftrax.com/goodies/gift-certificate
Comment #3
torgospizzaI'll change the title here, and make "Use purchaser's email if no recipient provided" another task / feature req.
Comment #4
torgospizzaSpelling fix.
Let me know what you think of creating this attribute, I think it'd allow us to simplify some of the logic.
Comment #5
jrust commentedI do think we can and should make Recipient Email not required. I think we should keep to a minimum the number of programatically added attributes, because every store is unique (see our store for how we set it up with a bunch of extras and varprice: http://www.pacificaperfume.com/pacifica-gift-certificate ) and we don't want to limit or enforce things that aren't needed.
But, here's my thought: have you noticed in the new version, on the settings page, it allows you to select which attribute is the Recipient Email? Well, maybe we just need another setting where you select which attribute is used to identify a Gift Certificate? By default it would be Recipient Email, but in the case of your store you could set it to dollar amount? Seems kind of kludgy, but that's the best I can come up with right now...
Comment #6
torgospizzaYeah, I like this idea. Because I kind of also like the ability to just have $5 certificate product, a $25 certificate product, etc. In fact I might revisit how we're doing it on our site, see which denominations are the most popular, and maybe that'll allow us to do away with using a "dollar amount" attribute.
Part of me is thinking about the way the original module was written, where you had to add a "gift certificate" product feature. To me that seems somewhat more intuitive than requiring store owners to choose an attribute that denotes whether a product is a certificate or not, but still feels a bit awkward. (As you said, it does seem a little kludgy).
Maybe this is best suited to creating a "Gift Certificate" product type / class? We could have the module define the gift_certificate node type.
Although then the issue becomes, What if the store owner wants to create a secondary gift certificate node type? You'd still need a way to say, "This is a certificate product." Unless others feel that there will always be one (and only one) certificate type, but you can create multiple products, some of which are printed, some of which are electronic, etc.
Comment #7
jrust commentedYeah, attributes are definitely add-on hack. I like the idea of product class -- isn't that how something like product kits is done? We should look at the product kits code and see if they allow for multiple node types of product kits.
Comment #8
jrust commentedThinking about it more, I think maybe the "product feature" ability might be the right way and would allow us to make all attributes optional.
Comment #9
torgospizzaI'll have to look through old uc_gift_certificate code, because IIRC, it started out this way. But then, I don't know if it was written very well (or robustly). Should we work on this for 1.x, or think about it as a 2.x feature?
Comment #10
jrust commentedAs long as it doesn't break backwards compatibility, I'm fine with 1.x