There should be a generic 'order comment' message log. End users should be able to created order comments for orders using the admin user interface. This plugin would simply have a template of 'Comment: {{ string }}' and the form provides the value for string'

CommentFileSizeAuthor
#141 2908196-Customer-facing-order-comments-141.patch36.66 KBayalon
#139 2908196-Customer-facing-order-comments-139.patch36.65 KBayalon
#138 2908196-Customer-facing-order-comments.patch37.08 KBayalon
#130 Screen Shot 2563-07-19 at 08.34.56.png56.44 KBabx
#128 2908196-128.patch34.85 KBgeertvansoest
#125 2908196-125.patch16.8 KBmglaman
#121 2908196-121.patch16.76 KBmglaman
#120 Screen Shot 2020-07-07 at 2.44.34 PM.png139.52 KBmglaman
#119 Screen Shot 2020-07-07 at 2.19.26 PM.png78.41 KBmglaman
#119 Screen Shot 2020-07-07 at 2.19.16 PM.png104.18 KBmglaman
#119 2908196-119.patch15.33 KBmglaman
#118 2908196-118.patch10.56 KBmglaman
#114 add_ability_to_add_an_order_comment-2908196-114.patch8.37 KBTylerMarshall
#112 add_ability_to_add_an_order_comment-2908196-112.patch8.13 KBTylerMarshall
#103 add_ability_to_add_an_order_comment-2908196-102.patch12.28 KBluksak
#100 add_ability_to_add_an_order_comment-2908196-100.patch33.75 KBsiegrist
#96 interdiff_87-96.txt1.87 KBshabana.navas
#96 add_ability_to_add_an_order_comment-2908196-96.patch33.86 KBshabana.navas
#92 2908196-92.patch26.89 KBravi.shankar
#90 add-ability-to-add-an-order-comment-2908196-90.patch26.82 KBayalon
#89 2908196-commerce_log_comments.patch35.74 KBayalon
#87 add-ability-to-add-an-order-comment-2908196-87.patch33.73 KBnavkaur
#87 interdiff-2908196-79-87.txt1.17 KBnavkaur
#86 add-ability-to-add-an-order-comment-2908196-86.patch45.44 KBnavkaur
#86 interdiff-2908196-79-86.txt1.17 KBnavkaur
#85 interdiff.2908196-79-83.txt1.17 KBnavkaur
#83 add-ability-to-add-an-order-comment-2908196-83.patch80.34 KBnavkaur
#83 interdiff.2908196-79-83.txt1.17 KBnavkaur
#80 interdiff.2908196.74-79.txt3.17 KBnavkaur
#79 add-ability-to-add-an-order-comment-2908196-79.patch33.65 KBnavkaur
#78 log-entry.jpg51.16 KBgge
#74 2908196-67.patch35.14 KBquietone
#71 2908196-Order-Comments.patch35.47 KBayalon
#67 2908196-67.patch35.14 KBquietone
#66 interdiff.2908196.64-66.txt1.34 KBNils Loewen
#66 2908196-66.commerce.Add-ability-to-add-an-order-comment.patch34.79 KBNils Loewen
#64 2908196-64.patch35.1 KBquietone
#64 interdiff-62-62.txt2.33 KBquietone
#62 interdiff-60-62.txt1.12 KBquietone
#62 2908196-62.patch32.77 KBquietone
#60 2908196-60.patch32.9 KBquietone
#60 interdiff-58-60.txt1.2 KBquietone
#58 interdiff-55-58.txt4.9 KBquietone
#58 2908196-58.patch32.89 KBquietone
#55 interdiff.2908196.52-54.txt9.12 KBNils Loewen
#55 2908196-54.commerce.Add-ability-to-add-an-order-comment.patch31.03 KBNils Loewen
#53 order_comment-2908196-52.patch31.71 KBNils Loewen
#51 Screenshot_2019-01-24 5 d8.png83.73 KBNils Loewen
#51 commerce-customer-view-order.png61.32 KBNils Loewen
#50 order_comment-2908196-50.patch31.34 KBNils Loewen
#50 interdiff__45-50.txt42.39 KBNils Loewen
#49 progress.png46.86 KBNils Loewen
#45 interdiff_43-45.diff7.77 KBNils Loewen
#45 commerce_pos--order-comments-45.patch27.7 KBNils Loewen
#44 interdiff-41-42.diff9.24 KBNils Loewen
#44 commerce_pos--order-comments-42.patch27.77 KBNils Loewen
#42 interdiff-40-41.diff781 bytesNils Loewen
#42 commerce_pos--order-notes-41.patch24.63 KBNils Loewen
#41 ordermessages.png3.33 KBNils Loewen
#39 commerce-add-order-comments-2908196-40.patch24.62 KBsafallia joseph
#35 commerce-add-order-comments-2908196-35.patch26.61 KBsafallia joseph
#34 2908196-34.patch31.56 KBsafallia joseph
#25 2908196-23-25.interdiff.txt13.75 KBcarlxjs
#25 2908196-25.patch31.5 KBcarlxjs
#23 2908196-22-23.interdiff.txt1.36 KBcarlxjs
#23 2908196-23.patch32.09 KBcarlxjs
#22 2908196-21-22.interdiff.txt3.21 KBcarlxjs
#22 2908196-22.patch30.73 KBcarlxjs
#21 2908196-20-21.interdiff.txt645 bytescarlxjs
#21 2908196-21.patch31.96 KBcarlxjs
#20 2908196-19-20.interdiff.txt421 bytescarlxjs
#20 2908196-20.patch31.56 KBcarlxjs
#19 2908196-16-19.interdiff.txt6.7 KBcarlxjs
#19 2908196-19.patch31.14 KBcarlxjs
#16 commerce-add-order-comments-2908196-15.patch26.55 KBgmem
#16 interdiff-commerce-add-order-comments-2908196-14.patch2.03 KBgmem
#15 interdiff-commerce-add-order-comments-2908196-13.patch455 bytesgmem
#15 commerce-add-order-comments-2908196-14.patch25.78 KBgmem
#13 commerce-add-order-comments-2908196-10.patch25.9 KBgmem
#13 interdiff-commerce-add-order-comments-2908196-9.patch439 bytesgmem
#9 interdiff-commerce-add-order-comments-2908196-8.patch14.64 KBgmem
#9 commerce-add-order-comments-2908196-9.patch25.9 KBgmem
#8 commerce-add-order-comments-2908196-8.patch21.33 KBgmem
#7 commerce-add-order-comments-2908196-7.patch16.54 KBgmem
#2 add_ability_to_add_an_order_comment-2908196-2.patch3.77 KBMaikel
#3 order_log_demo.png70.95 KBMaikel
#4 commerce-add-order-comments-2908196-4.patch9.46 KBzengenuity

Issue fork commerce-2908196

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

Maikel created an issue. See original summary.

Maikel’s picture

Started building a solution (or actually a colleague of mine did @dannypeeters). It's not ready yet but eager to share progress, and especially getting help with creating a better solution. Please let us know if we are going in the right direction.

Maikel’s picture

StatusFileSize
new70.95 KB

Short summary of what is going on;
- Still have to validate the form
- Content of mail is very basic. Message + Regards. Order content is available but not printed yet.
- Check if texts and comments are ok and provide correct defaults in english

Current working version screenshot attached.

zengenuity’s picture

I think the previous patch was incomplete. It was missing the new files created.

In any case, I picked up from the previous patch, reorganized a few things to better match how other parts of Commerce are designed, and implemented this again. It's not finished yet, but I need this for a project that I'm working on, so I want to push the ball a bit further down the court. Missing from this patch:

  • Sending email to the customer. I have a checkbox where the admin can decide if they want to do that or not, but it has no effect except to change the displayed message on the admin order view.
  • No permissions for who can add order comments.
  • Have not updated the customer order template to display the customer comments.
  • We'll need a checkout pane eventually to allow customer to enter a comment on checkout.

My project has to be finished in 2-3 months, so I should have all of those items completed by then.

joshmiller’s picture

Component: Order » Log

Commerce Point of Sale needed this and McCabe decided to implement a commerce_log based version of order comments and that patch landed a little while ago:

#2923195: Add Notes

The plan for Commerce, at least according to BojanZ through the comments of the above issue, was to then bring the work done in POS into Drupal Commerce.

joshmiller’s picture

Related issues: +#2923195: Add Notes
gmem’s picture

Assigned: Unassigned » gmem
StatusFileSize
new16.54 KB

This patch makes the checkbox for emailing the customer functional, still working on the other missing bits from #4 however.

gmem’s picture

Issue tags: +Needs tests
StatusFileSize
new21.33 KB

Working on the customer-facing note input, I'm not entirely sure about the wording and where I need to put the entry for the schema to actually install the notes column but regardless.

  • Implements pane form for the customer's comments.
  • Adds column to commerce_order to store the note since they'll only enter it once.
  • Adds getter/setter for the note in the order entity.

Since I've added it to the database and order entity, we could probably make it display elsewhere in the actual order page, but right now it logs just as the admin notes would with bold text to stand out. Displaying the notes in the customer's view of their order would be good too, it does display fine in the review however.

Need to work on permissions and test coverage.

gmem’s picture

StatusFileSize
new25.9 KB
new14.64 KB

Worked a bit on permissions, I'm working out the best way to test the new functionality, but regardless here what's new:

  • Worked on permissions, might need some work though.
  • Added customer notes and messages to their order view.

Moving to needs review for feedback.

gmem’s picture

Status: Active » Needs review

The last submitted patch, 4: commerce-add-order-comments-2908196-4.patch, failed testing. View results

The last submitted patch, 9: commerce-add-order-comments-2908196-9.patch, failed testing. View results

gmem’s picture

Fixed phplint error

Status: Needs review » Needs work

The last submitted patch, 13: commerce-add-order-comments-2908196-10.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

gmem’s picture

Removed schema from another unrelated issue that was accidentally added to the diff and added a dependency to commerce_log to commerce_checkout for the customer order notes.

gmem’s picture

Added commerce_log as a dependency to commerce_order as well, which should clean up the rest of the test errors. Also added a placeholder for the note input if the customer doesn't have the permission to add a comment, I wonder if it would be better to just add the hidden attribute to the element instead?

gmem’s picture

Status: Needs work » Needs review
skyredwang’s picture

Status: Needs review » Needs work

commerce_log entity doesn't use entity_reference to commerce_order? Why custom code?

carlxjs’s picture

StatusFileSize
new31.14 KB
new6.7 KB
carlxjs’s picture

StatusFileSize
new31.56 KB
new421 bytes
carlxjs’s picture

StatusFileSize
new31.96 KB
new645 bytes
carlxjs’s picture

StatusFileSize
new30.73 KB
new3.21 KB
carlxjs’s picture

StatusFileSize
new32.09 KB
new1.36 KB

Add a entity reference of commerce_order when save a commerce_log that its source_entity_type is 'commerce_order' .
Delete the entity reference of commerce_order when delete a commerce_log that its source_entity_type is 'commerce_order' .

bojanz’s picture

This patch should not be modifying the Order entity nor adding any order fields.

carlxjs’s picture

StatusFileSize
new31.5 KB
new13.75 KB

Removed the entity reference field that I added in commerce_order.
Change commerce_log field source_entity_type and source_entity_id to be a entity reference field source_entity.

bojanz’s picture

There's a reason why we didn't use an entity_reference field. ER requires you to specify an entity type, a log can be created for any entity type. So your approach won't work. Use what you have.

skyredwang’s picture

a log can be created for any entity type

This came up in our previous discussion. Why is that? is there a use case for commerce_log to be attached to e.g.node? Shouldn't commerce_log assume it would only work with commerce_order?

bojanz’s picture

There is a use case for logs produced by payments, for example.

Besides, changing this would be both a BC break and require a large and impractical update function.

flocondetoile’s picture

I am using commerce log for orders (of course, with custom implementation), but for payments too, and to handle a global notification system (as the message stack could do it) for some users on changes made on products, nodes and even some taxonomy terms.

So the commerce logs entities on one of my projects are related to order, payment, product, node and term.

skyredwang’s picture

There is a use case for logs produced by payments, for example.

Good to know.

ER requires you to specify an entity type, a log can be created for any entity type.

Just to clear, ER can be created for any entity type, too. But, for a specific field instance, ER can target one entity type. But, I don't see useful use-cases that a commerce_log entity can target multiple entity types?

Besides, changing this would be both a BC break and require a large and impractical update function.

I thought this patch is being reviewed, therefore, no production support yet. But, if you already have sites, using this on production, you might just commit the code first, then future commits will try to minimal tweak the code?

bojanz’s picture

Just to clear, ER can be created for any entity type, too. But, for a specific field instance, ER can target one entity type. But, I don't see useful use-cases that a commerce_log entity can target multiple entity types?

The target_type is set on the Log entity type level (in the base field definition).

I thought this patch is being reviewed, therefore, no production support yet. But, if you already have sites, using this on production, you might just commit the code first, then future commits will try to minimal tweak the code?

The patch is modifying the Log entity, used by every existing Commerce site, hence the BC comment.

safallia joseph’s picture

Assigned: gmem » safallia joseph

I cloned the latest commerce, and patch in #25 is not applying cleanly for me in branch 8.x-2.x.
I'm getting errors

error: patch failed: modules/order/commerce_order.install:73
error: modules/order/commerce_order.install: patch does not apply

I'm trying to re-roll the patch to work against the latest code

skyredwang’s picture

The patch is modifying the Log entity, used by every existing Commerce site

Ah, I missed understood the situation. Just to confirm, this issue is still following the plan in #5. I will look into the "notes"

safallia joseph’s picture

StatusFileSize
new31.56 KB

Re-rolled the patch in #25 to work against the latest code base.
Looking into the issues mentioned by Bojanz in #26

safallia joseph’s picture

Status: Needs work » Needs review
StatusFileSize
new26.61 KB

Removed the entity reference added for source entity in patch #25

safallia joseph’s picture

Assigned: safallia joseph » Unassigned

Status: Needs review » Needs work

The last submitted patch, 35: commerce-add-order-comments-2908196-35.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

krystalcode’s picture

I did a review of this issue and patch.

Firstly I wanted to voice the opinion that going with a log-based approach brings along the following problems:

- Default UX is poor; for most users notes/comments on an order is arguably something different than a list of log entries.
- UI more difficult to customize, even though possible.
- Not flexible/extensible enough, it would require more effort to build application-specific functionality on top of it.

More flexibility and ease would be provided by using comments.

That said and assuming that this has been discussed, here's a few comments after testing the last patch.

- Checkout assumes that the reason that someone does not have permission is that they're not logged in; this might not always be the case. It might be better to hide the pane if the user has no permission instead of displaying a message that you need to log in to add a note to the order.
- Notes added on checkout are added to a field. From Bojan's comment "This patch should not be modifying the Order entity nor adding any order fields.", are we clear on how we're implementing this?
- As a customer user I cannot add a note to my orders even after having the right permissions assigned. Is the idea only for admins to send messages to the customer? That'd be pretty limiting.
- In the checkout pane's validatePaneForm function: remove if there's no validation to add?
- The event subscriber is defined in both services file and in service provider. The definitions added in the service provider do not seem to do anything that can't be done with the static definition in `services.yml` file so I'd suggest they are removed for both event subscribers.
- Flush all caches: you need to look into invalidating the order entity's cache tags specifically.

On the `CommerceLogOrderMessageForm.php` class:

- Current user service should be injected in the constructor.
- You can make the buildForm function more readable by doing if (!\Drupal::currentUser()->hasPermission('add notes to order')) { return; } at the top.

safallia joseph’s picture

@krystalcode - Thanks for the review

- Checkout assumes that the reason that someone does not have permission is that they're not logged in; this might not always be the case. It might be better to hide the pane if the user has no permission instead of displaying a message that you need to log in to add a note to the order.

Made the pane visible only for users who has permission `add notes to own order`

- Notes added on checkout are added to a field. From Bojan's comment "This patch should not be modifying the Order entity nor adding any order fields.", are we clear on how we're implementing this?

Removed the order fields and used commerce log entity

As a customer user I cannot add a note to my orders even after having the right permissions assigned. Is the idea only for admins to send messages to the customer? That'd be pretty limiting.

Now users who has the permission `add notes to own order` is able to add comments to their orders

In the checkout pane's validatePaneForm function: remove if there's no validation to add?

Done - Removed the validation

- The event subscriber is defined in both services file and in service provider. The definitions added in the service provider do not seem to do anything that can't be done with the static definition in `services.yml` file so I'd suggest they are removed for both event subscribers.

That is correct, removed the unwanted CommerceLogServiceProvider.php File

Flush all caches: you need to look into invalidating the order entity's cache tags specifically.

Invalidated only order caches

On the `CommerceLogOrderMessageForm.php` class:

- Current user service should be injected in the constructor.
- You can make the buildForm function more readable by doing if (!\Drupal::currentUser()->hasPermission('add notes to order')) { return; } at the top.

- Done and Done

Please review the attached patch

safallia joseph’s picture

Status: Needs work » Needs review
Nils Loewen’s picture

StatusFileSize
new3.33 KB

Review of patch 40:

1:When going through checkout, you can write an order comment, but if you go back and try to edit order comment, the previous message is lost. Every time a message is submitted a new entry is created, it should probably only save when the order is completed.

2: Anonymous customer using 'Checkout as Guest' does not have permission to write an order comment by default. I apologies if I have missed previous discussion about this. Is it intended that the logs are only for admin and logged in users? Either we should make this visible to all users and remove the permission, or permission should be given to anon and authenticated users on commerce install.

3. Adding messages is presented as a feature when view a user's orders `/user/1/orders/4`. However, the submit button does not work. Also The previous messages should be presented with at least a time stamp.
Display options should be available at /admin/commerce/config/order-types/default/edit/display/user for order logs.

Nils Loewen’s picture

StatusFileSize
new24.63 KB
new781 bytes

Changed the default permissions to allow Anon customers to write order comments.

Status: Needs review » Needs work

The last submitted patch, 42: interdiff-40-41.diff, failed testing. View results

Nils Loewen’s picture

StatusFileSize
new27.77 KB
new9.24 KB

Reorganized OrderEventSubscriber. Split onCustomerMessage emailing process into sendCustomerMsgEmail() function as per the TODO.

Also includes phpcs and dependency injection fixes for that file.

Nils Loewen’s picture

StatusFileSize
new27.7 KB
new7.77 KB

Cleaned up code in OrderEventSubscriber.
Changed dependencies of commerce_cart needs commerce_log because of local installation issues. The commerce_log.services.yml was looking to commerce_cart content before commerce_cart was installed. I don't know why the D.o testrunner worked at all.

Nils Loewen’s picture

Issue summary: View changes

[Comment Deleted]

Nils Loewen’s picture

+++ b/modules/log/src/EventSubscriber/OrderEventSubscriber.php
@@ -195,51 +194,46 @@ class OrderEventSubscriber implements EventSubscriberInterface {
+    $event->stopPropagation();

This may have unintended consequences. It was necessary because the log messages were being saved twice.

Nils Loewen’s picture

deleted

Nils Loewen’s picture

StatusFileSize
new46.86 KB

TODO: The Order Comments are not being displayed to user when user views order details. Ideally the Order Log messages would be their own field managed by Order Type, 'User' view mode.

Also, I intended to differentiate log messages with 'To Customer' and 'From Customer'. However, when a customer adds a message after placing an order, the wrong heading comes up. See image:

Nils Loewen’s picture

StatusFileSize
new42.39 KB
new31.34 KB

Changes in this interdiff:

- Unified the naming of this new feature. Technically we are (now) using the Log entity to store a property called 'comment'. Previously this patch referred to these messages as 'notes', 'messages', 'comments', and 'log messages'. I have renamed everything to call this new feature a 'comment'. This naming is open to debate. It is certainly important for the code to be consistent. The biggest downside to this choice is that it competes with a 'comment' from Drupal proper.

- Differentiated Event TYPE_CUSTOMER into TYPE_TO_CUSTOMER and TYPE_FROM_CUSTOMER and their respective functions and templates.

- Changed permissions to add/view admin comments, add/view customer comments.
- Enabled these permissions on install with commerce_log.install

- UX changes, see next comment.

Nils Loewen’s picture

Issue summary: View changes
StatusFileSize
new61.32 KB
new83.73 KB

[deleted]

Nils Loewen’s picture

Nils Loewen’s picture

StatusFileSize
new31.71 KB

Tests are failing due to dependency issues between commerce sub-modules.

I will try resolve this by using hooks for commerce_log to alter commerce_order.

travis-bradbury’s picture

commerce_log_install() should probably not be duplicating permissions from commerce_checkout_install(). Maybe take the comments ones out of commerce_checkout_install() and take the 'access checkout' ones out of commerce_log_install().

Nils Loewen’s picture

joshmiller’s picture

+++ b/modules/checkout/src/Plugin/Commerce/CheckoutPane/CustomerCommentsPane.php
@@ -28,18 +28,17 @@ class CustomerCommentsPane extends CheckoutPaneBase implements CheckoutPaneInter
+    if (\Drupal::service('module_handler')->moduleExists('commerce_log')) {
...
+      $log_storage = \Drupal::entityTypeManager()->getStorage('commerce_log');

@@ -49,12 +48,14 @@ class CustomerCommentsPane extends CheckoutPaneBase implements CheckoutPaneInter
+    if (\Drupal::service('module_handler')->moduleExists('commerce_log')) {

@@ -66,18 +67,16 @@ class CustomerCommentsPane extends CheckoutPaneBase implements CheckoutPaneInter
+    if (\Drupal::service('module_handler')->moduleExists('commerce_log')) {

+++ b/modules/order/commerce_order.module
@@ -124,21 +121,26 @@ function template_preprocess_commerce_order(array &$variables) {
+  if (\Drupal::service('module_handler')->moduleExists('commerce_log')) {
...
+    $log_storage = \Drupal::entityTypeManager()->getStorage('commerce_log');

Dependency Injection please :)

quietone’s picture

Assigned: Unassigned » quietone

Interested in moving this along. Will do fixes for #56.

quietone’s picture

Status: Needs work » Needs review
StatusFileSize
new32.89 KB
new4.9 KB

Fixes for #56 and used inheritdoc on the methods in the same file.

Status: Needs review » Needs work

The last submitted patch, 58: 2908196-58.patch, failed testing. View results

quietone’s picture

Status: Needs work » Needs review
StatusFileSize
new1.2 KB
new32.9 KB

I'm not able to run these tests locally now, so am relying on testbot.

Fix the constructor and changed a line for phplint, so it works in Php 5.

Status: Needs review » Needs work

The last submitted patch, 60: 2908196-60.patch, failed testing. View results

quietone’s picture

Status: Needs work » Needs review
StatusFileSize
new32.77 KB
new1.12 KB

Now 2 items picked up with phpcs

Status: Needs review » Needs work

The last submitted patch, 62: 2908196-62.patch, failed testing. View results

quietone’s picture

Assigned: quietone » Unassigned
Status: Needs work » Needs review
StatusFileSize
new2.33 KB
new35.1 KB

Add commerce_product to several tests, hoping it will fix these errors.

1) Drupal\Tests\commerce_tax\FunctionalJavascript\CustomTest::testTaxTypeCustom
Drupal\Core\Config\UnmetDependenciesException: Configuration objects provided by commerce_cart have unmet dependencies: core.entity_view_mode.commerce_product_attribute_value.add_to_cart (commerce_product)

Status: Needs review » Needs work

The last submitted patch, 64: 2908196-64.patch, failed testing. View results

Nils Loewen’s picture

I tried to take a quick look and see what's happening with these tests. I could not find a quick fix.

This Interdiff is just a nit of something I missed when converting all 'message' instances to 'comment' for consistency.

Aside from fixing the tests, the biggest todo for this issue is to reduce sub-module interdependecies. Ideally the chunk of code in order.module::preprocess could be moved to log.module. Maaaybe we could add an `@import` in the commerce-order--user.html.twig for the order_comments section which would point to a template in commerce_log.

quietone’s picture

StatusFileSize
new35.14 KB

Needed a reroll.

quietone’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 67: 2908196-67.patch, failed testing. View results

ayalon’s picture

Unfortunatly the patch #68 cannot be applied to Commerce 2.12. @quietone

ayalon’s picture

StatusFileSize
new35.47 KB

Here is a reroll that applies to 2.12

jseniuk’s picture

Status: Needs work » Reviewed & tested by the community

Patch above works

quietone’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

@jseniuk, Hi welcome to Drupal! Nice to know the patch worked for you. What steps did you take to verify that it worked?

This needs to go back to NW because it still needs tests and it needs a reroll. The patch is not applying.

quietone’s picture

StatusFileSize
new35.14 KB

Just fixing whitespace errors in the patch.

I've taken a brief look at the failing tests and confirmed I don't know enough about commerce and Functional tests to make a fix. Anyone able to take a look?

quietone’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 74: 2908196-67.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

gge’s picture

When the Orders Comments box is placed on the Order Information, some errors are displayed when on the next step, in my case Order Review page and the customer comments is an empty panel, only the title is shown.
The errors:
User error: "0" is an invalid render array key in Drupal\Core\Render\Element::children() (line 97 of core/lib/Drupal/Core/Render/Element.php).
User error: "1" is an invalid render array key in Drupal\Core\Render\Element::children() (line 97 of core/lib/Drupal/Core/Render/Element.php).
#74

When the Orders Comments box is placed on a different pane (tried on Review) it seems to work fine.
My PHP version is 7.3.4.

Also when trying to apply the patch got this:

patching file modules/log/commerce_log.info.yml
Hunk #1 FAILED at 5.
1 out of 1 hunk FAILED -- saving rejects to file modules/log/commerce_log.info.yml.rej

commerce_log.info.yml.rej contain this:

--- modules/log/commerce_log.info.yml
+++ modules/log/commerce_log.info.yml
@@ -5,3 +5,5 @@ package: Commerce
 core: 8.x
 dependencies:
   - commerce:commerce
+  - commerce:commerce_cart
+  - commerce:commerce_order

Thanks

gge’s picture

StatusFileSize
new51.16 KB

Also, when there is no comment added by the customer, there should be no entry created in the Order Log.

navkaur’s picture

The patch in #74 works, just needed few changes.
When doing a fresh build of site, we can't have a service that depends on a module, cause each service is executed when the container is built
as mentioned, https://www.drupal.org/project/commerce/issues/2839426
so added services back in CommerceLogServiceProvider.php

navkaur’s picture

StatusFileSize
new3.17 KB

interdiff for patch #79

navkaur’s picture

luksak’s picture

The patch works perfectly for me! Thank you!

navkaur’s picture

The Renderer strips the newline tags from "Comment" textfield (which is of type = textarea) and sends email to customer as one big string. This happens when a comment contains multiple new lines. (comment with "customer facing comment " option and "notify customer by email" checked).
This new patch converts every new line into new string for email body.

navkaur’s picture

navkaur’s picture

StatusFileSize
new1.17 KB

Correct interdiff

navkaur’s picture

StatusFileSize
new1.17 KB
new45.44 KB

Apologies, the above patches #83 and #86 are not correct.

navkaur’s picture

StatusFileSize
new1.17 KB
new33.73 KB

Correct patch is #87.
Sorry for all the mess.

luksak’s picture

I kept getting errors when commerce orders entities were rendered on routes that do not have a commerce_order parameter. In my case that happened when generating pdfs using entity_print. This is the error I got:

The website encountered an unexpected error. Please try again later.
Error: Call to a member function id() on null in Drupal\commerce_log\Form\CommerceLogOrderCommentForm->buildForm() (line 101 of modules/contrib/commerce/modules/log/src/Form/CommerceLogOrderCommentForm.php). 

This is the quick fix i came up with:

public function buildForm(array $form, FormStateInterface $form_state) {
    // Proceed only if user has permission to add order comments and the current
    // page has a commerce_order parameter.
    $orderId = $this->routeMatch->getParameter('commerce_order');
    $addAdminComment = $this->account->hasPermission('add admin comments on order');
    $addCustomerComment = $this->account->hasPermission('add customer comments on order');
    if (!$orderId || (!$addAdminComment && !$addCustomerComment)) {
      return NULL;
    }
ayalon’s picture

StatusFileSize
new35.74 KB

Here is a reroll with the changes Lukas von Blarer suggested:

ayalon’s picture

StatusFileSize
new26.82 KB

My previous patch had issues being applied. Here is a fixed version.

codebymikey’s picture

How close/feasible is this feature to landing on an actual release?

It's extremely useful functionality, especially when migrating from an existing system with order notes.

ravi.shankar’s picture

StatusFileSize
new26.89 KB

I have re-rolled patch #90. It still needs tests.

jastraat’s picture

With the latest patch, I see the following error after attempting to add an admin comment on an order:

Warning: call_user_func() expects parameter 1 to be a valid callback, class 'Drupal\commerce_log\EventSubscriber\OrderEventSubscriber' does not have a method 'onAdminComment' in Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch()

The patch adds the following to getSubscribedEvents() in OrderEventSubscriber:

'commerce_order.comment.to_customer' => ['onToCustomerComment', -100],
'commerce_order.comment.from_customer' => ['onFromCustomerComment', -100],
'commerce_order.comment.admin' => ['onAdminComment', -100],

But there are no functions that correspond to these.

shabana.navas’s picture

+++ b/modules/checkout/src/Plugin/Commerce/CheckoutPane/CustomerCommentsPane.php
@@ -0,0 +1,121 @@
+    return \Drupal::currentUser()->hasPermission('add comments to own order');

currentUser() should be injected.

shabana.navas’s picture

Okay, apparently, the patch was good up until #87. Somehow, the patch that was submitted starting in #89 has been messed up and omits some of the changes including the changes added to the OrderEventSubscriber class in commerce_log.

shabana.navas’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
StatusFileSize
new33.86 KB
new1.87 KB

Rerolled #87, which was the last good patch, for latest 8.x-2.x and added changes in #89.

jastraat’s picture

Unfortunately that last patch doesn't apply with composer

However - it does apply to the latest dev with git without any errors. And so far in testing it works to allow both comments on checkout and administrative comments after checkout. Thanks!

shabana.navas’s picture

Yeah, I've add the same composer issue with the last few patches, including mine. It's just not applying w/ composer but works fine with git and no issues reported.

ayalon’s picture

The patch is renaming a file and this does not work with Composer (Issue of cweagans/composer-patches).

If you want to use the patch with Drupal Commerce 2.16, you will have to force Composer to checkout the Git repo. Then the patch applies cleanly.

The commit-hash for Commerce 2.16 is "29024144":

"require": {
    "drupal/commerce": "2.x-dev#29024144",
}

Then you can use the patch with composer.

siegrist’s picture

Reroll for 2.17.

Status: Needs review » Needs work

The last submitted patch, 100: add_ability_to_add_an_order_comment-2908196-100.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

luksak’s picture

The patch in #100 doesn't apply using composer. Here is a try to fix that.

luksak’s picture

Status: Needs work » Needs review
StatusFileSize
new12.28 KB

Status: Needs review » Needs work
luksak’s picture

Hm, somehow I am failing to fix the patch for composer as well... It applies using git.

luksak’s picture

I am sorry, my issue was caused by not using latest dev...

luksak’s picture

I wanted to point out this feedback in #41:

1:When going through checkout, you can write an order comment, but if you go back and try to edit order comment, the previous message is lost. Every time a message is submitted a new entry is created, it should probably only save when the order is completed.

I also think this is crucial, since the customers would be confused if their comment is missing when they visit the checkout step that holds the comment pane again and their comment is not visible. I have an earlier version of this patch in production and a lot of customers fill out the comment multiple times, which is pretty confusing for the people that handle the orders.

Also editing a comment should be possible and since in my opinion only one comment should be possible during checkout, we should always load the customer's existing comment in the checkout pane.

What do you think?

joekers’s picture

Does anyone have any info on how to use the core comment system on orders rather than using the commerce log functionality? Maybe there was a different issue for that and some reasons why that approach wasn't used.

Using commerce log doesn't give the flexibility I need.

rszrama’s picture

@joekers out of scope for this issue, but maybe a good conversation thread for Slack. It's not something we'll be looking to do in core.

@Lukas von Blarer, if I'm not mistaken, your patches are leaving out quite a bit ... basically every new file. Did you mean to do that?

rszrama’s picture

Having reviewed the patch in #100, some thoughts:

  1. We need to do a full review of the interface text. It's not there yet.
  2. The new checkout pane should be disabled by default. Most sites don't need random order comments on checkout, and keeping it disabled by default means we don't have to worry about module updates suddenly changing checkout forms.
  3. The checkout pane docblocks are confused, referring to the billing information pane, CompletionRegister object, etc.
  4. If the checkout pane just has a textarea in it, I don't see why its wrapper element should be a fieldset. Can we not just make this a container div?
  5. We should review the log templates; I'm not sure the use of HTML like that is standardized in Commerce contrib yet, but we can also look at recent core commits that added order email logging to core.
  6. Why are we adding "access checkout" permissions in the commerce_log module? Seems out of place / out of scope, no?
  7. As previously stated, we shouldn't be creating customer comments until they complete checkout (i.e. the order is placed). You can just stash the user input in the order's data array and then create the log on checkout completion.
  8. Why are we adding commerce_log_mail()? I didn't see that discussed up above.
  9. I'm not sure I understand the OrderCommentEvent implementation. What's the rationale for overloading a single event like this vs. making separate events? And as is, it wouldn't differentiate between order comments left in checkout vs. on the order view page.
  10. I really don't understand the whole implementation of actually adding comments and comment forms to orders. I expected to see at least an "extra field" I could move around / disable, but nope ... there's no way to hide the comments feature once this patch is enabled. That's a major dealbreaker. Furthermore, this is only exposed via a default Twig template - what if a theme has provided its own? And why not use Views to create the table of comments shown? This smells like it was basically one site's very specific implementation.
  11. No tests. Deal breaker.

All that said, the patch and approach aren't bad, it's just not a usable patch that needs a lot of love from someone really invested in the current approach before we can consider moving it into core. Frankly, I think we stand a much better chance of narrowing the focus back down to solving the original post: adding comments to orders from the admin interface via log messages.

That represents a much more achievable scope where we can solve some of the issues I've pointed out above, and then we can open a separate feature request specifically for things like defining a checkout pane, emailing admin comments, making comments visible on the order view page, etc.

idflorin’s picture

I look forward to it. This will be in dev|stable or separate module?
1 month, 2 months?

TylerMarshall’s picture

Status: Needs work » Needs review
StatusFileSize
new8.13 KB

This patch sets out to simplify the scope of this issue:

Users with permissions to view commerce orders, and with the new permission of "Add admin comment to orders" should be able to go to any order view page, and have the ability to add an Admin comment (specified via template).

rszrama’s picture

Status: Needs review » Needs work

@TylerMarshall Rock on, this is exactly what I envisioned. If I may make a small change request, I'm thinking we should make OrderCommentForm even more generic than I originally specced out so we can use it in other contexts more easily later (e.g. a checkout pane). To do that, I think we need the following changes:

  1. Update OrderComment::buildForm() to require a $log_template_id argument that determines which template gets used in the submit handler for adding the comment to the order.
  2. Let's move the textarea description from the form builder and to AdminOrderCommentForm::render() and update the calling of the form builder to specify the order_admin_comment log template. Oh, and when we do, let's change "other users" to "users" in the description so it doesn't read like the comment may not be visible to the person making it.

Good find on nl2br(), btw! Didn't know that func existed. : D

TylerMarshall’s picture

Status: Needs work » Needs review
StatusFileSize
new8.37 KB
rszrama’s picture

Status: Needs review » Needs work

@Tyler The logic in the Views area handler is incorrect:

    if ($empty || $this->options['empty']) {
      return [];
    }

The area handler should actually have the "empty" option enabled by default (meaning the comment form should be visible even if there are no log entries on the order yet), and the if statement should rather be:

    if ($empty && empty($this->options['empty'])) {
      return [];
    }

I was trying to figure out why the area handler wasn't showing for me, and it's because I'd configured the area handler to appear on empty, but the || made it invisible. : D

Other suggestions:

  1. Let's change the default template to bold "Admin comment:" and then to add a line break after it so the admin comment starts on the next line.
  2. When the title of the fieldset is clicked, let's use JavaScript to focus on the comment textarea.
  3. It appears we aren't sanitizing tags on input our output, meaning any HTML tag is rendered. I think for the scope of this issue, I'd prefer to just support line breaks and to filter all other tags out. We can always change our approach in the future if we wanted to do something like support text format selection on input. (I don't know if there's a Twig based approach to just support br and p tags or some other way to do output filtering; if it's not possible, we can just filter on input and change our approach later if need be.)
  4. We should add an optional confirmation message to the form, but since we were trying to make the base form reusable, it wouldn't make sense to do it there. We should be able to just add a submit handler to the form AdminOrderCommentForm::render() and have it set the message, "Comment saved." on submission.

I'm curious to know what options we'd have for an upgrade path here, too. We can just document that existing sites who want this feature will need to enable it, but it would be great if we had some interface or update hook for adding the area handler to the View. Any ideas?

mglaman’s picture

Assigned: Unassigned » mglaman

Reviewing and working on this to push it over the edge!

mglaman’s picture

I created an issue fork and put #114 on it: https://git.drupalcode.org/issue/commerce-2908196/-/commit/fc75ad4f61c62...

#115 shows we need automated test coverage.

I'm curious to know what options we'd have for an upgrade path here, too. We can just document that existing sites who want this feature will need to enable it, but it would be great if we had some interface or update hook for adding the area handler to the View. Any ideas?

If no one modified modules/log/config/install/views.view.commerce_activity.yml , we can use our ConfigUpdater service to import the new changes. We can also add instructions to the issue summary on how to add this.


+++ b/modules/order/commerce_order.permissions.yml
@@ -2,3 +2,8 @@
+
+'add admin comments to orders':
+  title: 'Add admin comments to orders'
+  description: 'Provides the ability to add admin order comments to an order.'
+  'restrict acces': TRUE

It feels weird to have this permission here, when it is actual a log action.

The Log module should be providing this permission. And it should actually provide it for any supported entity, yeah? Since logs can belong to any entity type.

The commerce_log_categories definitions let us know what entities have log templates available.


  1. +++ b/modules/log/src/Form/OrderCommentForm.php
    @@ -0,0 +1,92 @@
    +    $comment = $form_state->getValue('comment');
    

    We need to pass to \Drupal\Component\Utility\Xss::filter

  2. +++ b/modules/log/src/Form/OrderCommentForm.php
    @@ -0,0 +1,92 @@
    +    $comment = t(nl2br($comment));
    

    Why are we passing to t? We're accepting user input

  3. +++ b/modules/log/src/Plugin/views/area/AdminOrderCommentForm.php
    @@ -0,0 +1,88 @@
    +  protected $orderStorage;
    ...
    +    $this->orderStorage = $entity_type_manager->getStorage('commerce_order');
    ...
    +      if ($order = $this->orderStorage->load($argument->getValue())) {
    

    We've started to move away from putting entity storage handlers as properties, since it adds work to the serialization process of forms. We can just inject the entity type manager and fetch the storage when needed (which is a performance boost as well.)

  4. +++ b/modules/log/src/Plugin/views/area/AdminOrderCommentForm.php
    @@ -0,0 +1,88 @@
    +    $current_user = \Drupal::currentUser();
    +    $permission = $current_user->hasPermission('add admin comments to orders');
    ...
    +        $form = \Drupal::formBuilder()->getForm('\Drupal\commerce_log\Form\OrderCommentForm', $order, 'order_admin_comment');
    

    Needs injection

mglaman’s picture

Status: Needs work » Needs review
StatusFileSize
new10.56 KB

Here's a new WIP patch.

mglaman’s picture

#115.2

When the title of the fieldset is clicked, let's use JavaScript to focus on the comment textarea.

So the click to expand is native browser interaction with a details element. So we want to add a JavaScript snippet which does:

const details = document.querySelector('[data-drupal-selector="edit-log-comment"]');
details.addEventListener('click', () => {
    document.querySelector('[data-drupal-selector="edit-comment"]').focus();
});

Here are screenshots, as well.

mglaman’s picture

Status: Needs review » Needs work
StatusFileSize
new139.52 KB

I uploaded the wrong screenshot before, here it is:

I did some discussing with @rszrama. The generated permission should say "Add admin comments to order", but my work to make this apply to all entities essentially added admin-only comment support to them without identifying it that way. Working out how we could support collecting customer

I'm going to rename commerce_order_comment to commerce_order_admin_comment. So the Log module will check for {$entity_type_id}_admin_comment and support creating comments from an activity log. In a follow up we can figure out how to use \Drupal\commerce_log\Form\LogCommentForm to submit non-backoffice comments.

mglaman’s picture

Assigned: mglaman » Unassigned
Status: Needs work » Needs review
StatusFileSize
new16.76 KB

Here's the change: https://git.drupalcode.org/issue/commerce-2908196/-/commit/4033b6c8c1ced...

This keeps the LogCommentForm generic, but the Views area handler specific to admin comments.

TylerMarshall’s picture

Status: Needs review » Reviewed & tested by the community

Reviewed, changes make sense to me.

mglaman’s picture

  1. +++ b/modules/log/src/CommentPermissions.php
    @@ -0,0 +1,74 @@
    +class CommentPermissions implements ContainerInjectionInterface {
    

    Should this be "LogCommentPermissions" or just "LogPermissions"

  2. +++ b/modules/log/src/CommentPermissions.php
    @@ -0,0 +1,74 @@
    +        $permissions["add ${entity_type_id} admin log comments"] = [
    

    This permission name feels weird, but it's also never exposed.

    add commerce_order admin log comments

mglaman’s picture

Okay, internally we discussed and settled on add commerce_log ${entity_type} admin comment for the permission. Going to roll that patch quick and then commit if tests are green!

mglaman’s picture

StatusFileSize
new16.8 KB

Okay, updated patch for sanity check before commit.

Diff: https://git.drupalcode.org/issue/commerce-2908196/-/commit/d204820e5ebe5...

  • mglaman committed 12cb8ca on 8.x-2.x
    Issue #2908196 by mglaman, TylerMarshall, rszrama: Add ability to add an...
mglaman’s picture

Status: Reviewed & tested by the community » Fixed

🥳 Committed!

geertvansoest’s picture

StatusFileSize
new34.85 KB

Despite the last patches are looking very good, we're not able to implement it in our current shop. If doing so, we'll lose some main features that already are in our shop, so we've to elaborate on patch #100. I'm the colleague of Maikel, who opened this issue back in 2017, and still have to maintain the same shop he was developing right then.

After upgrading to the last Drupal (8.8.8) and Commerce (2.20) version, I wasn't able to implement #100 anymore. So I made some small fixes, after which I was able to implement it again.

Unless I know this version needs much improvements and will not be in the "core" in short term, I've attached it. Maybe some other people who're facing the same problems can still use this.

mglaman’s picture

@geertvansoest your patch adds customer comments and allowing them to be made from the checkout form, correct? Trying to discern. This should definitely move into a new issue so we can review and work on it. It'll make it easier to review your patch.

abx’s picture

StatusFileSize
new56.44 KB

@mglaman, patch #128 also add "Customer-facing comment" that an admin can use in each order to communicate with customers by commerce log and to send an Email to customers if needed

*** Ubercart 2 (Drupal 6) has this right out of the box and this is what I'm searching how to do in Commerce 2 since we need to contact our customers from time to time and leave a note so that all of our admin know what is happening.

Patch #128

Status: Fixed » Closed (fixed)

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

geertvansoest’s picture

@mglaman It's true what @abx says. Those functionalities where in some patches posted in this ticket earlier, I just made them (patch #100) compatible with the latest Drupal and Commerce version.

ayalon’s picture

@geertvansoest: Is there a chance, that you upgrade the patch again for Commerce 2.21? We are also stuck on this patch and I appreciated your upgrade a lot!

geertvansoest’s picture

@ayalon I will, if we update Commerce to 2.21 ourselves. At this moment we're still using Commerce 2.20.

mhmhartman’s picture

Just adding my 2 cents here, it's partly related and might save some people a lot of time.

Some people probably only need a simple comment field where the customer can add a note for the admin. You can add fields through /admin/commerce/config/shipment-types/default/edit/fields and display them to the customer and admin. No other actions needed.

Update: forgot to mention i'm using version 2.20

pacifigraphic’s picture

@mhmhartman In version 2.23, the path / admin / commerce / config / shipment-types / default / edit / fields has been replaced by / admin / commerce / config / order-types ... this does not work, no possibility to display the comment field for customers, and patch # 125 is already up to date on version 2.23. I can't find the solution to simply display a comment field in the review page.

afzal hussain’s picture

In version 2.25 the core comment module can use for commerce product, order etc by creating a targeted comments types.

ayalon’s picture

Here is a reroll of the missing features of this thread with "Customer Facing" comments with mail notifications.
Patch applies against Commerce 2.26. Based on #128

ayalon’s picture

Here is a reroll for Commerce 2.28 with renamed permissions:

morbus iff’s picture

@ayalon: Could you make a new issue with your updated patch, per #129?

ayalon’s picture

Yes we should separate it.

Meanwhile another reroll for Commerce 2.29.

luksak’s picture

This issue is closed. Should we open a new one for the functionality that is included in the patch of #141 since a few people including me need that?

vipin.j’s picture

I have split the issue and created a new related one for this as per requested with #140 and #142. Have also updated & uploaded the patch based on #141.

abx’s picture

anybody’s picture

https://www.drupal.org/project/commerce_checkout_order_fields might also be helpful in cases like this? Leaving the link FYI!

morbus iff’s picture

@Anybody: we use both, aye. That module can work for "when the customer leaves the admin a comment"; this issue (and #3267366) handles the other end - an admin responding back to a customer about their order.

nags338228’s picture

Version: 8.x-2.x-dev » 8.x-2.38

Hi guys,

I am not able to apply #141 patch for 2.38 or lower versions having error: "The process "patch '-p0' --no-backup-if-mismatch -d".

Can anyone lead some light please?

Thanks !

morbus iff’s picture

nags, please see #143, where future development is happening.

nags338228’s picture

Hi @Morbus,

I patched with https://www.drupal.org/project/commerce/issues/3267366 and https://www.drupal.org/project/commerce/issues/2908196 this issue and merged both of it.

The #25 patch contains Comment form in user order page for the user along with permissions + comment form for admin with radio buttons and sending email functionality. It supports the latest version of Commerce module which is 2.38

please do have a look if you have some time.

Thanks !!