1. The CommerceProductCRUDTestCase::testCommerceProductTokens() could be simplified with a copy of the assertTokens function from Token.module rather than running token_replace() N times.
2. The CommerceBaseTestCase still references entity_tokens as a dependency when it is no longer necessary for any modules nor tests.
3. The [commerce-product:creator] token should be using format_username() by default when it is not.
4. There is no [commerice-product:url] token when there probably should be. This can take advantage of token.module's 'url' token type if token.module is installed.

Comments

dave reid’s picture

Status: Active » Needs review
Issue tags: +token
StatusFileSize
new7.9 KB
dave reid’s picture

I'm also now very confused as to why the [commerce-product:edit-url] token actually works...

rszrama’s picture

Cool, any patch that cleans up our tests gets a huge +5 from me. I don't see any problem with the tests as they are, but I probably need to move the Product URL token to a commerce_product_ui.token.inc file. We don't actually define any entity URI for our entities in their base modules but do it in their UI modules where an entity info alteration takes place to point to the appropriate URLs.

One problem we may have with a Product URL token is that it's always going to point to the admin URL of the product. We don't actually define a canonical front-end URL. I think I had a URL type distinction for Orders on the front-end (i.e. under an account tab) vs. the back-end (i.e. from the Orders admin View), but I don't see it in the Order UI Token file any more... just the front-end one. Not sure what happened there or if we should have a token for both. You have any thoughts?

Thanks for the patch!

rszrama’s picture

Ahh, heh, just saw your follow-up comment. Yeah, it's coming from Product UI. ; )

chris matthews’s picture

Issue summary: View changes
Status: Needs review » Needs work
Issue tags: -token +Needs reroll

The 7 year old patch in #1 does not apply to the latest commerce 7.x-1.x-dev and (if still relevant) needs to be rerolled.

bojanz’s picture

Status: Needs work » Closed (outdated)