Question: Do we really need Bundles(PriceListTypes) for the PriceList entity? I think we should remove them. I don't see the use case for price list bundles.

1) The PriceList has some basefields for which there are no corresponding methods in the interface. As far as I see these are the missing methods in PriceListInterface

::setStartDate()
::getStartDate()
::setEndeDate()
::getEndDate()

2) We attach two fields to the default price list bundle during install.

One is the price_list_item field and the other is the stores field. The first one is essential a basefield as we define a getter in the interface. So we have to make sure, that we attach this field to all newly generated PriceListTypes. We cannot use a basefield here, because views integration is broken in core. But we have a pattern in commerce core, e.g. how the stores or variations fields are added. Setter(s) is missing in the interface, too.

Stores field shouldn't be attached at all. If we want to restrict a pricelist per store, we should do it with conditions.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

olafkarsten created an issue. See original summary.

olafkarsten’s picture

The patch doesn't introduces any heavy conceptual changes. It's about cleanup, simple fixes of parameter names and adding missing
pieces in the interface and implementation, as well as fixing the doc comments.

- Adds missing conditions base fields
- Adds missing condition getters and setters to price list and interface
- Adds missing start and end date methods
- Adds sort and getDefaultStartDate helper methods
- Adds the available helper method, that checks whether a price list
is enabled and the current date is in the range start date <> end date.
- Adds the weight method to both, the price list type and the interface
- Use enabled / disabled instead of published, as commerce core does in such cases
(like promotions)
- Adding the missing start date and end date methods
- Adding some more labels in price list entity definition
- Makes price lists translatable
- Adds commerce cores access handler to price list type
- Using drupals DefaultHtmlRouteProvider for price list types. No need to provide an own
handler that duplicates the code without any relevant changes.
- Delete PriceListTypeHtmlRouteProvider ^^
- Change the admin permission in price list type to meet commerce core standard
- Let the PriceListTypeDeleteForm extending CommerceBundleEntityDeleteFormBase, because it
prevents deletion if it used somewhere.

ToDo:
- decide whether to remove the bundles (price list types)
- decide whether to remove the stores reference field

lawxen’s picture

Status: Active » Needs review

I just select parts of 2966977-PriceList-Cleanup-2.patch and make some other non-functional-change.

Let's handle other thing in several standalone issues and don't gather them to one big issue :)

  1. weight
  2. condition
  3. user_id => uid , this needs hook_update
lawxen’s picture

some change about pricelist entity definition has been change in https://www.drupal.org/project/commerce_pricelist/issues/2966977#comment...

The last submitted patch, 2: 2966977-PriceList-Cleanup-2.patch, failed testing. View results

  • caseylau committed 895369f on 8.x-2.x authored by olafkarsten
    Issue #2968238 by olafkarsten: Cleanup PriceList, PriceListType and...
lawxen’s picture

Status: Needs review » Fixed

  • caseylau committed 5901f8e on 8.x-2.x
    Issue #2968238 by olafkarsten, caseylau: Delete...

Status: Fixed » Closed (fixed)

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