Move the variations form to its own tab next to (Product) Edit

Liking this proposal. It is a simple approach that hooks in to Drupal Core functionality.

A couple of years ago we used this successfully in a Drupal 7 Commerce website.
There it was quite natural to split editing of the product-content (then the 'display') and the stock + availability settings of variations. We used the module Editable Views to make this more user-friendly. (See attached image)

The usability challenge (how I see it) between creating and editing product+variations won't be solved totally. For creating there is still room for a wizard-like method.
For the editing-part this proposal will make customisation (by developers) way more easy and extendible then Inline Entity Form.

Here is an initial version of a patch that separates the variations onto their own tab, following the model of promotions and coupons.

It works, for the most part, but with a few things missing:

1. The IEF form for single-variation product types is still a complex cardinality-unlimited type. I'm not sure what we should do here. We can switch it to simple IEF style, but we still need to have it behave as cardinality=1. I don't think we can switch the field cardinality itself without major rework. Not sure what the best approach for this issue is.

2. It's still possible to restore the IEF form element on product types with multiple variations by going to Manage Form Display and dragging it out of the Disabled section. But maybe it's okay if the site builder chooses to do that, it's their choice.

3. With this change, it's possible to create a product with no variations. That doesn't break anything, but it obviously doesn't work in practice either, which could be confusing for users.

4. No tests: need to add at least one to confirm that the tab is displayed and hidden based on the product type option.

5. No upgrade code: We need to hide the Variations field from all product types on upgrade to keep it consistent with new types created. We'd have to assume all existing types could have multiple variations, unless we want to query to confirm that. This is going to change the UI for all existing sites, which seems like a big deal.

Read through the patch zengenuity,

My thoughts (not a comprehensive review)
- Boilerplate for routing is there, great
- The variation-listing looks good, but when other things are done we should switch to a View
- The 'single/multiple variations' setting is there
- The creation of a first product-variation(s) is not there

Regarding:

1. The IEF form for single-variation product types is still a complex cardinality-unlimited type. I'm not sure what we should do here. We can switch it to simple IEF style, but we still need to have it behave as cardinality=1. I don't think we can switch the field cardinality itself without major rework. Not sure what the best approach for this issue is.

Do I understand you right when 'single-variation product types' is used, we still have a unlimited field-cardinality?
My initial feeling is to delay the 1:1 implementation till the 1:many is done.

2. It's still possible to restore the IEF form element on product types with multiple variations by going to Manage Form Display and dragging it out of the Disabled section. But maybe it's okay if the site builder chooses to do that, it's their choice.

Both the current and the new approach in principle will work for both 1:1 and 1:many implementations. So for this behavior this is ok to me. I see the new approach as an alternative interface.

3. With this change, it's possible to create a product with no variations. That doesn't break anything, but it obviously doesn't work in practice either, which could be confusing for users.

drugan did a lot of work on bulk-creation. I am very interested about his opinion how we should do this!

4. No tests: need to add at least one to confirm that the tab is displayed and hidden based on the product type option.

If move the development to github, we can leverage travis and the that makes testing a lot easier.
The impact of this change is quite huge, we need functional test coverage (clicking the new UI).
Thoughts:
- Variations tab visibiliy (as you said)
- Add/edit/create variation
- Local-tasks, local-actions on variations
- Views implementation (for example sorting) to validate Views is working

5. No upgrade code: We need to hide the Variations field from all product types on upgrade to keep it consistent with new types created. We'd have to assume all existing types could have multiple variations, unless we want to query to confirm that. This is going to change the UI for all existing sites, which seems like a big deal.

Feel like this change should be opt-in first. Especially for existing sites. Note this development approach is also used in the Paragraphs module with their experimental widget.

IMO would help if drugan can jump in

I've tested the patch and for me it looks as a great starting point. A bit later I am going more deeply to explore it and see what could be done on this patch and the variation bulk creator patch to make them work together.

This looks like a feature that is going to need careful testing and user-experience polling.

Previous patch did not fully clean up the entity reference when a variation was deleted from a product from the Variations tab. This patch fixes that problem, and it's also a re-roll against current dev.

Previous patch did not do access control correctly for product variations. Prevented add to cart form from being displayed to normal users. This patch fixes that.

I like the idea in general, but now that 2.0 is out this is going to break contrib & custom code that has any expectations on the product variation form being where it currently is.

For example, in https://www.drupal.org/project/commerce_license_og_role the License type plugin configuration form uses AJAX, and that makes assumptions about the IEF form structure in order to know whether it's in a PV form for a new entity or an existing one.

> It has served us well, but it has also constrained the things we can do to variations as a result.

I'll say also, one limitation of IEF is that you can customize the columns you show in the summary, but not as much as you'd like (I don't remember the fine details... is it maybe that you can customize columns per entity type, but not per bundle, which means for Commerce that basically all PVs show the same?)

A really nice feature for this issue would be to allow each Product Type to specify the view display it uses for its view of PVs.

I too like the idea. It'd be nice to see a similar implementation for order items for all the same reasons given in the issue summary.

I just took a quick look and have one comment:

I think the variations listing needs to be draggable so that the variations can be ordered.

I also like the work done by @zengenuity but seems that it is more appropriate for a contrib, otherwise we should wait forever. Without knowing the maintainer mind on the patch it is quite difficult to participate as it may happen that you waist your time for some useless work.

Again: time is money.

I'm wondering whether it would be ok to simplify things by changing the "allow multiple variations" setting to a "use variations tab" setting...

* On the Product Type edit form, change the checkbox wording
from: "Products of this type can have more than one variation."
to: "Use a separate tab for editing product variations."

* Add description/help text: "For products with multiple variations, we suggest using the variations tab for data entry and disabling the Variations field using Manage Form Display."

* Do not disable the checkbox after the product type is initially created. Allow users to choose tab/no-tab at any point.

As @ndf mentioned (#4),

I see the new approach as an alternative interface.

Why not implement it as an optional feature?
If it should be the default (with the assumption that multiple variations are more common than single-variation products), then the config for the Default product type that's installed by default could have the "Use a separate tab..." option selected.

I think this helps with @joachim's comments (#10),

this is going to break contrib & custom code that has any expectations on the product variation form being where it currently is.

If you want to use a contrib module that assumes the ief widget for variations, then you still have that option.

Undoubtedly, I'm overlooking key details with my thoughts here on simplifying the variations tab implementation, but I thought I'd just see whether this could be an option...

(What would be even cooler, perhaps, would be a way to specify the "variations tab" option as a Widget choice for Variations on the Manage Form Display page, with the widget somehow behaving on the Product edit form as if the Variations field was disabled and the access control somehow using the selection of widget to determine whether to show the variations tab. Besides the "somehows", it also seems like it's a bad idea, if even possible, to abuse the field widget plugin system like that.)

Rejoining this pity party with a re-roll of the patch. :) I don't think this patch applies against 2.3, but the patch in #9 doesn't apply against dev. This re-roll does, and it cleans up some .orig files I let slip into the last patch.

I agree with most of the comments above that it's hard to see how this won't break contrib modules. So, I'm not optimistic about the patch's acceptance. But, I also have a project going live soon that requires this functionality, so I'm stuck maintaining this to some degree. I'm not sure if it's possible to do everything we're doing here as contrib module. Probably. However, if I'm going to be maintaining this myself as a hack, I prefer keeping it as a patch, since a patch fails loudly in composer on Commerce update. If there's more consensus about how to move forward and maintain it as contrib, then I could go that way.

It would work for me to switch this to a "use variations tab" setting rather than limiting the cardinality for using IEF, as lisastreeter suggested. The key thing for me is I need to be able to edit variations on their own form, accessed from a list on another page. I have products with 50+ variations and growing, so IEF is a not going to work for my use case.

> I agree with most of the comments above that it's hard to see how this won't break contrib modules. So, I'm not optimistic about the patch's acceptance.

The proposal in #14 would help with that. Those contrib modules that expect the IEF form can simply say in the documentation that that is required (or check for it in settings form validation).

> If it should be the default (with the assumption that multiple variations are more common than single-variation products), then the config for the Default product type that's installed by default could have the "Use a separate tab..." option selected.

I like this idea.

> What would be even cooler, perhaps, would be a way to specify the "variations tab" option as a Widget choice for Variations on the Manage Form Display page, with the widget somehow behaving on the Product edit form as if the Variations field was disabled and the access control somehow using the selection of widget to determine whether to show the variations tab.

I'm similarly torn between thinking this is kind of nifty, but also an abuse of the Field UI... :)

Quick and incomplete patch review:

1. +++ b/modules/product/commerce_product.routing.yml
   @@ -5,3 +5,17 @@ commerce_product.configuration:
   +entity.commerce_product_variation.collection:
   

   There's a route provider for this entity type, so the route should be defined there.

2. +++ b/modules/product/src/Access/ProductVariationCollectionAccess.php
   @@ -0,0 +1,29 @@
   +class ProductVariationCollectionAccess implements AccessInterface {
   

   Missing class docs.

3. +++ b/modules/product/src/Access/ProductVariationCollectionAccess.php
   @@ -0,0 +1,29 @@
   +   * A custom access check.
   

   Needs to say something useful!

Found a little bug in the ProductVariationListBuilder class. If a product attribute is not required (and not entered for a variation) or If you create a new product attribute for a variation type after you've already created variations for that type, the buildRow() method of the ProductVariationListBuilder class won't work.

$entity->getAttributeValue($field_name) is FALSE for the product attribute, so this statement fails:

$row[$field_name] = $entity->getAttributeValue($field_name)->label();

+++ b/modules/product/src/Form/ProductVariationForm.php
@@ -0,0 +1,62 @@
+      $values += [
+        'type' => $product_type->getVariationTypeId()
+      ];

I don't want to creep the scope, but once we have this UI we could consider as a followup allowing product types to hold product variations of more than one type.

@bojanz's comment on #2880602: One Product types for many Product variation types suggests that limitation was done to keep the UI of the inline entity form simple.

I could not get either patch #15 or patch #9 to apply, so I don't have an interdiff here. I addressed @joachim's patch review comments and made the variations tab optional to avoid affecting existing contrib modules.

Also, I had some issues related to the ProductVariationAccessControllerHandler and existing test code (since variations can no longer be assumed to be embedded in the product form.) As a result, I made a few modifications to Kernel tests. (See the very end of the patch.) The access controller handler and changes to the Kernel tests definitely need a review by somebody who understands Entity Access better than I do.

Also needed is additional test code to cover this new functionality...

Eyeball review which is mostly just formatting nitpicks:

1. +++ b/modules/product/src/Form/ProductVariationForm.php
   @@ -0,0 +1,61 @@
   +class ProductVariationForm extends ContentEntityForm {
   

   Missing class docblock.

2. +++ b/modules/product/src/Form/ProductVariationForm.php
   @@ -0,0 +1,61 @@
   +      // If the entity has bundles, fetch it from the route match.
   +      $entity_type = $this->entityTypeManager->getDefinition($entity_type_id);
   +      if ($bundle_key = $entity_type->getKey('bundle')) {
   

   We know that product variations have bundles.

3. +++ b/modules/product/src/ProductVariationAccessControlHandler.php
   @@ -0,0 +1,47 @@
   +   */  ¶
   

   Stray whitespace.

4. +++ b/modules/product/src/ProductVariationAccessControlHandler.php
   @@ -0,0 +1,47 @@
   +        // Grant access, assuming the product access control handling has already happened.
   

   Comment should be wrapped to 80 chars.

5. +++ b/modules/product/src/ProductVariationListBuilder.php
   @@ -0,0 +1,127 @@
   +    	}
   +    	else {
   +        $row[$field_name] = $this->t('(Missing attribute value)');
   +    	}
   +    }
   

   Looks like there are tabs here.

6. +++ b/modules/product/src/ProductVariationRouteProvider.php
   @@ -0,0 +1,61 @@
   +  /**
   +   * Gets the collection route.
   +   *
   +   * @param \Drupal\Core\Entity\EntityTypeInterface $entity_type
   +   *   The entity type.
   +   *
   +   * @return \Symfony\Component\Routing\Route|null
   +   *   The generated route, if available.
   +   */
   

   Shouldn't this be an inheritdoc too?

Rerolled patch. Resolved a conflict in commerce_product.services.yml

@joachim. Thank you for the code review. Made changes suggested in comment #21

This patch makes the product variation list draggable so that variations can be reordered.

Thanks!! I love this. Did some manual testing.

- Patch applies
- option to enable the tab works / IEF still there
- disable the tab kind of works. Tab remains visible until cache is cleared. You get an 403 if you try to access it, after it is disabled.
- adding / editing / deleting / rearrange of product variations works
- all this works even if you switch between tab and ief (variations field is not disabled of course)
- adding new attribute or make one not required works

> - disable the tab kind of works. Tab remains visible until cache is cleared. You get an 403 if you try to access it, after it is disabled.

To fix that we need to clear some caches when the product type is saved and the setting is changed. Router cache & the various menu link / action / task plugins.

I think $this->routeBuilder->rebuild(); will work, and it causes action/tasks plugins to refresh, too. I think needsRebuild might be appropriate.

routeBuilder->rebuild() didn't work.

Had some success with adding a custom cache tag in the ProductVariationCollectionAccess handler and invalidating this cache tag when the product type form is saved.

Found this idea on stackexchange

Seems to work. But TBH I'm not that familiar with the cache system and don't know if this is an appropriate approach.

Code looks like
ProductVariationCollectionAccess::access

public function access(AccountInterface $account, ProductInterface $commerce_product) {
    $product_type = ProductType::load($commerce_product->bundle());
    $cache_tag = ['product_variation_tab:' . $product_type->id()];
    return $commerce_product->access('update', $account, TRUE)
      ->andIf(AccessResult::allowedIf($product_type->shouldShowVariationsTab()))
      ->addCacheTags($cache_tag);
  }

ProductTypeForm::save

if ($status == SAVED_UPDATED) {
        $cache_tag = ['product_variation_tab:' . $this->entity->id()];
        \Drupal\Core\Cache\Cache::invalidateTags($cache_tag);
}

I think the proper place to put tags that require invalidation on entity CRUD is EntityInterface::getCacheTagsToInvalidate()?

Regarding the various cache ideas, I'm wondering whether it would be acceptable to just display a, "hey, you need to rebuild caches" message? End-users aren't going to be disabling the tab. Non-admin users won't even be using it. And it doesn't seem unreasonable (to me) to ask site builders to clear caches. I can't imagine that switching back-and-forth between the ief and variations tab would be done on a site with any sort of frequency. Messing around with caches in the code seems like just one more thing that needs testing and one more thing that could break. And there's not too much to be gained for the effort.

(Please feel free to tell me I'm completely wrong here!)

IMHO you're wrong, maybe not completely though and it's certainly a good question.

There's a good case to be made for not walking into a known WTF condition, and we can't be the first module contributors/maintainers who have run into this kind of issue. Looking at the convo on this ticket so far it just seems like we need to find the right place(s) to do the invalidation. Testing it is also trivial once we figure that out.

I have a client with a real interest in cleaning up the variations UI so I will see if I can't get some budgeted time to do that and work on this issue to help get it over the finish line, as well.

Ah, it's only just occurred to me that this is exactly the same problem I dealt with in https://www.drupal.org/project/entity_ui.

See https://cgit.drupalcode.org/entity_ui/tree/src/Entity/EntityTab.php#n267 for the relevant code.

I think a hard clear of the whole cache is appropriate here, as this is an admin operation, and a rare one that would typically only be done during the initial site building.

@bradjones1 and @joachim Thanks for the responses and push forward. The EntityTab code was a big help! I experimented a little and found that clearing the routing and local tasks caches didn't do anything but clearing the render cache worked. So that's all I've done in this patch. Please let me know if I should be doing more clearing.

Oops. Patch was missing a use statement. Also fixed a few little coding standards issues here.

Getting closer... I've made the changes in #38 and added test code.

Just removed unused use statement and fixed a couple coding standards issues.

Hello - Any plans to re-roll this patch for Commerce 2.8?

I applied patch # 37 but I don't see any changes. Any help!

ok, update on this. #37 is the last patch that applies cleanly to commerce 2.8. I was able to get the tab to show up after checking the checkbox on the product type config page (@lukasss this might help you).

However, I do have one question. Is this patch supposed to create a view for the variation tab so you can add filters or bulk operations?

Poking along at this during Drupal Europe.

+++ b/modules/product/src/Entity/ProductType.php
@@ -43,6 +44,7 @@ use Drupal\commerce\Entity\CommerceBundleEntityBase;
+ *     "showVariationsTab",

@@ -77,6 +79,13 @@ class ProductType extends CommerceBundleEntityBase implements ProductTypeInterfa
+  /**
+   * Indicates whether variations tab should be provided for edit form.
+   *
+   * @var bool
+   */
+  protected $showVariationsTab = FALSE;
+

@@ -122,4 +131,31 @@ class ProductType extends CommerceBundleEntityBase implements ProductTypeInterfa
+   */
+  public function shouldShowVariationsTab() {
+    return $this->showVariationsTab;
+  }

We decided to just always show the variations tab and not support the old way.

Later we can come up with a better UX when the variations do not have attributes (ie: single product, single variation)

Here's a reduced patch, leaving us to do the following:

* Set the variations field to hidden/disabled.
* Stop passing ?destination when clicking on "Variations" from a product operations link. It causes a redirect to the product collection after adding/editing variations

This adds the upgrade hook, which properly removes the component. Need to look into the ?destination item.

This should fix the tests. It removes old IEF assumptions. I think our code coverage is still OK because of the new tests that were written. Probably worth a follow up to review ProductAdminTest and beef it up and triage it.

Tomorrow I'll go over it more to evaluate the tests, and the patch. For example I just noticed

+++ b/modules/product/src/Form/ProductForm.php
@@ -26,6 +27,13 @@ class ProductForm extends ContentEntityForm {
+  /**
+   * The current user.
+   *
+   * @var \Drupal\Core\Session\AccountInterface
+   */
+  protected $currentUser;
+

@@ -37,11 +45,14 @@ class ProductForm extends ContentEntityForm {
+   * @param \Drupal\Core\Session\AccountInterface $current_user
+   *   The current user.
...
+  public function __construct(EntityManagerInterface $entity_manager, EntityTypeBundleInfoInterface $entity_type_bundle_info, TimeInterface $time, DateFormatterInterface $date_formatter, AccountInterface $current_user) {
...
+    $this->currentUser = $current_user;

@@ -52,7 +63,8 @@ class ProductForm extends ContentEntityForm {
+      $container->get('current_user')

We don't need this anymore. Was added due to access check created in the previous patch.

This patch fixes that dead code, along with some other small changes.

I did realize that ProductVariationRouteProvider removed the entity create access check, relying only on the product update check. Some of this work was done before 2.9 when access control was improved, so we need test coverage. Access to a variation collection should only be allowed if the user has admin/manage permissions for variations and the ability to update the product.

While writing some tests, I realized we have a slight problem. Without #2953566: Allow entities to specify a "collection permission" existing in core, viewing the Variations tab requires the admin permission for variations...which is administer commerce_product. That kind of breaks the ability to restrict product management to users who own their product, by giving them a very powerful permission (breaks marketplace style setups)

We have manage {bundle} commerce_product_variation permissions, but at the collection level we do not know the bundle. We could allow viewing the variations tab by access of updating the product alone, which then edit/delete on that page are restricted by permissions.

The same issue exists on the add form, The following permissions are required: 'administer commerce_product' OR 'manage commerce_product_variation'.. We don't know what bundle is being used, so the permission is broken.

This will fail, but adds testing for the route access.

We need to decide:

1. Should the entity.commerce_product_variation.collection route be controlled by the admin permission or update access on the product? I'm assuming the latter, as it prevents providing the administer permission to every user
2. The generated permission "manage $entity_bundle commerce_product_variation" is passed a null bundle on the add-form.

Perhaps ProductVariationRouteProvider overrides getAddFormRoute and getCollectionRoute to set the requirements as only ['_entity_access' => 'commerce_product.update']. This seems less messy than forcing a hand out of "administer commerce_product"

The add form was fixed by properly defining add-page and add-form. Discussed with bojanz and we decided the variation collection should use the access commerce_product overview permission. This means marketplace users would just need to add ['_entity_access' => 'commerce_product.update'] to restrict variation pages to product owners.

This patch should ensure route access and test it!

Forgot some phpcs fixes required:

1. +++ b/modules/product/src/Form/ProductForm.php
   @@ -10,6 +10,7 @@ use Drupal\Core\Entity\EntityTypeBundleInfoInterface;
   +use Drupal\Core\Session\AccountInterface;
   

   Unused

2. +++ b/modules/product/src/ProductVariationRouteProvider.php
   @@ -0,0 +1,79 @@
   +  protected function getDeleteFormRoute(EntityTypeInterface $entity_type) {
   +    $route = parent::getDeleteFormRoute($entity_type);
   

   Forgot inheritic doc

My last change wasn't the right way to go with the routes.

Okay, here is a fix. Instead of using _entity_create_access and fighting the way it passes a bundle to the create access check, I just provided a version called _product_variation_create_access which gets the variation type ID from the product type.

Discussed with bojanz, I forgot we dropped the secondary permission requirement (product access.) This is something marketplace users can easily add in.

Here is a new patch.

We should set the product_id in $values as well, since it is known. Same way we do it in PriceListItemForm.

Missed that, which allows us to remove

  /**
   * {@inheritdoc}
   */
  protected function prepareEntity() {
    if ($this->entity->isNew()) {
      $product = $this->getRouteMatch()->getParameter('commerce_product');
      $this->entity->set('product_id', $product);
    }
  }

We're missing the admin one.

Whoops. Added. And testing a user with only the admin permission.

Since we do not have a canonical route, the translation routes are not automatically generated for us. We'll need to build a translation UI for variations.

See content_translation_entity_type_alter:

      if ($entity_type->hasLinkTemplate('canonical')) {
        // Provide default route names for the translation paths.
        if (!$entity_type->hasLinkTemplate('drupal:content-translation-overview')) {
          $translations_path = $entity_type->getLinkTemplate('canonical') . '/translations';
          $entity_type->setLinkTemplate('drupal:content-translation-overview', $translations_path);
          $entity_type->setLinkTemplate('drupal:content-translation-add', $translations_path . '/add/{source}/{target}');
          $entity_type->setLinkTemplate('drupal:content-translation-edit', $translations_path . '/edit/{language}');
          $entity_type->setLinkTemplate('drupal:content-translation-delete', $translations_path . '/delete/{language}');
        }
        // @todo Remove this as soon as menu access checks rely on the
        //   controller. See https://www.drupal.org/node/2155787.
        $translation['content_translation'] += [
          'access_callback' => 'content_translation_translate_access',
        ];
      }

:( digging in, we'd need to basically copy most of \Drupal\content_translation\Routing\ContentTranslationRouteSubscriber::alterRoutes

Discussed with bojanz, and to reduce scope of this patch we'll do the translations as an immediate follow up.

This works greate for me!
It's much better!
Rather in production