There are some boilerplate comments and unnecessary stuff floating aruond from the kickoff commit. That needs a little bit love.
And we should use the EntityPublishedInterface and EntityPublishedTrait from core instead of writing own methods.

CommentFileSizeAuthor
#12 interdiff2966977-8-12.txt5.65 KBlawxen
#12 2966977-12.patch16.67 KBlawxen
#9 interdiff.txt3.2 KBolafkarsten
#8 2966977-PriceListItem-Cleanup-8.patch20.5 KBolafkarsten
#6 interdiff.txt4.04 KBolafkarsten
#6 2966977-PriceListItem-Cleanup-6.patch20.65 KBolafkarsten
#6 interdiff.txt4.04 KBolafkarsten
#4 2966977-PriceListItem-Cleanup-3.patch17.88 KBolafkarsten
#3 2966977-PriceListItem-Cleanup-2.patch16.54 KBolafkarsten
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

olafkarsten created an issue. See original summary.

olafkarsten’s picture

olafkarsten CreditAttribution: olafkarsten as a volunteer and at SimplE-Commerce commented
Issue summary: View changes
olafkarsten’s picture

olafkarsten CreditAttribution: olafkarsten as a volunteer and at SimplE-Commerce commented
Status: Active » Needs review
FileSize
2966977-PriceListItem-Cleanup-2.patch16.54 KB

Kick of patch:

Cleanup the PriceListItem and PriceListItemInterface:

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

- Using EntityPublishedInterface and EntityPublishedTrait from drupal core for handling the status
- Adding setPriceList method to interface and price list item. Mainly for DX. We should follow commerce
core practices as long as we don't have a good reason to not follow.
- For the same reason use the EntityOwnerInterface and implement the corresponding methods.
- Adding the missing start date and end date methods to interface and entity
- Adding some more labels in price list item entity definition
- Makes price list item translatable

olafkarsten’s picture

olafkarsten CreditAttribution: olafkarsten as a volunteer and at SimplE-Commerce commented
Title: Cleanup and Refactor the PriceList entity, PriceListType entity and Interfaces » Cleanup and Refactor PriceListItem, PriceListItemType and Interfaces
Issue summary: View changes
Related issues: +#2968238: Cleanup PriceList, PriceListType and Interfaces
FileSize
2966977-PriceListItem-Cleanup-3.patch17.88 KB

Nevermind #2

During cleanup for PriceList and PriceListType I stumbled about some inconsistencies in my patch above.

This patch
- Does not use the EntityPublishedInterface and EntityPublishedTrait. It's semantically cleaner to speak about enabling / disabling of an price list item instead of publishing.
- Removes the date fields. Start and end dates are setting of the price list, not the price list item.
- Removes the weight field. Price List Items are ordered by quantity (tiers) not weight.

Lets use this issue for PriceListItems only. Will open another one for the PriceList and Interface.

olafkarsten’s picture

olafkarsten CreditAttribution: olafkarsten as a volunteer and at SimplE-Commerce commented
olafkarsten’s picture

olafkarsten CreditAttribution: olafkarsten as a volunteer and at SimplE-Commerce commented
FileSize
interdiff.txt4.04 KB
2966977-PriceListItem-Cleanup-6.patch20.65 KB
interdiff.txt4.04 KB

Next iteration: adding conditions field and methods.

lawxen’s picture

lawxen CreditAttribution: lawxen at Sparkpad commented
Status: Needs review » Needs work

This module is chaotic, because it's code doesn't look like commerce. @olafkarsten this is a so good optimizing task. 

  1. +    $fields['condition_operator'] = BaseFieldDefinition::create('list_string')
+      ->setLabel(t('Condition operator'))
+      ->setDescription(t('The condition operator.'))
+      ->setRequired(TRUE)
+      ->setSetting('allowed_values', [
+        'AND' => t('All conditions must pass'),
+        'OR' => t('Only one condition must pass'),

    Condition_operator belong to commerce_promotion, it shouldn't be in price list item, Or I missed important thing? 

  2. +      ->setDescription(t('The time when the price list item was created.'))
+      ->setTranslatable(TRUE)
+      ->setDisplayOptions('form', [
+        'type' => 'datetime_timestamp',
+        'weight' => 10,
+      ])
+      ->setDisplayConfigurable('form', TRUE)
+      ->setDisplayConfigurable('view', TRUE);

    We have discussed this in our team,

    Start and end dates are setting of the price list, not the price list item.

    and we have deleted the Start and end dates in our use case through disable it on form display for a long time. Let's just make it same with product variation, and don't show it in edit form. 

  3. +    $fields['status'] = BaseFieldDefinition::create('boolean')
+      ->setLabel(t('Status'))
+      ->setDescription(t('Whether the price list item is enabled.'))
+      ->setDefaultValue(TRUE)
+      ->setRequired(TRUE)
       ->setSettings([
-        'datetime_type' => 'datetime',
-      ])
-      ->setDefaultValue('')
-      ->setDisplayOptions('view', [
-        'label' => 'above',
-        'type' => 'datetime_default',
-        'settings' => [
-          'format_type' => 'medium',
-        ],
-        'weight' => 6,
+        'on_label' => t('Enabled'),
+        'off_label' => t('Disabled'),
       ])
       ->setDisplayOptions('form', [
-        'type' => 'datetime_default',
-        'weight' => 6,
-      ])
-      ->setDisplayConfigurable('form', TRUE)
-      ->setDisplayConfigurable('view', TRUE);
+        'type' => 'options_buttons',
+        'weight' => 0,
+      ]);

    This will be better too if it keep same with productVariation.

olafkarsten’s picture

olafkarsten CreditAttribution: olafkarsten as a volunteer and at SimplE-Commerce commented
Status: Needs work » Needs review
FileSize
2966977-PriceListItem-Cleanup-8.patch20.5 KB

This module is chaotic, because it's code doesn't look like commerce.

Haha :D

I guess, so looks the start of projects. Throw in a bunch of ideas, see if it works and make it better :D

This will be better too, if it keep same with productVariation.

Regarding enabled/disabled of a price list item. Commerce core uses both. Active/Inactive on variation enabled/disabled on Promotion. Drupal uses more enabled/disabled: I personally think enabled/disabled fits better, but to be honest, its probably a matter of taste in this case. Only published doesn't fit semantically.

Condition_operator belong to commerce_promotion, it shouldn't be in price list item, Or I missed important thing?

If I understand bojanz proposal for the modules architecture right, #2966942: Architecture D8 Version (Meta) we want conditions on both - the price list and the price list item. As we than could have multiple conditions on price lists as well as on price list items, I would like to have an operator, too. If we really need conditions (and operators) on the price list item level, I don't know yet. At the moment I don't even know, if we should have the price list per purchasable entity (product variation) or on the product level. Thats why I created the meta issue. If these decisions are already made, they should at least be documented somewhere.

Attached patch adresses #2 and #3

ToDo:
Adjust all the forms for price list items to meet the data structures. But thats for another patch. Lets keep the steps small.

olafkarsten’s picture

olafkarsten CreditAttribution: olafkarsten as a volunteer and at SimplE-Commerce commented
FileSize
interdiff.txt3.2 KB

Sorry, forgot the interdiff for #8

skyredwang’s picture

skyredwang
Location Shanghai
CreditAttribution: skyredwang at INsReady for Sparkpad commented
Status: Needs review » Needs work

- Removes the weight field. Price List Items are ordered by quantity (tiers) not weight.

I don't understand why we would order the pricelist_items by quantity? A usual user story I understand would be: have two pricelists consisting products for two stores in different cities. One same product would show up as a featured product in Store A, while the product would show up at the bottom in Store B. Weight fields can be used to meet the user story, but if we change the ordering by quantity, then, is there another way to do this?

olafkarsten’s picture

olafkarsten CreditAttribution: olafkarsten as a volunteer and at SimplE-Commerce commented

Hmm, maybe we have a different understanding of price list and price list item. TBH I don't understand your user story. I don't know what "show up as featured product" means. That sounds like product listing - but that's nothing for price list items?

The price list item as I understand it - holds the price, the currency, the qty/tier, the variationid. It's job is to give us a unit price for a given qty, currency, no? Why should we order them by weight?

I guess we need to agree on some important points before going forward. I don't see any detailed plan and agreement on such fundamental stuff in the issue queue. Thats why I opened #2966942: Architecture D8 Version (Meta). Lets discuss us the issue there and come back if we have an common understand what we want to accomplish.

lawxen’s picture

lawxen CreditAttribution: lawxen at Sparkpad commented
FileSize
2966977-12.patch16.67 KB
interdiff2966977-8-12.txt5.65 KB

Let's delete all controversial change in this issue, and create new issue about them.

Diff with previous patch:

  1. Delete conditions
  2. Delete condition_operator
  3. Cancel the change of weight, though it don't take effect, we can solve it in new issue.
lawxen’s picture

lawxen CreditAttribution: lawxen at Sparkpad commented
Status: Needs work » Needs review

lawxen’s picture

lawxen CreditAttribution: lawxen at Sparkpad commented
Status: Needs review » Fixed

  • caseylau committed c6f4493 on 8.x-2.x 
    Issue #2966977 by olafkarsten, caseylau, skyredwang: Cleanup and...
lawxen’s picture

lawxen CreditAttribution: lawxen at Sparkpad commented

Last commit make price_list's base field stored in a standalone table too (price_list_field_data).

Status: Fixed » Closed (fixed)

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