Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue #2861664 by mglaman: Order assignment should generate a log to … #680

Merged

Conversation

mglaman
Copy link
Contributor

@mglaman mglaman commented Mar 17, 2017

…mark the customer conversion

@bojanz
Copy link
Contributor

bojanz commented Mar 20, 2017

The build failed because of phpcs:

FILE: ...e/commerce/modules/log/tests/src/Kernel/OrderIntegrationTest.php
----------------------------------------------------------------------
FOUND 1 ERROR AND 1 WARNING AFFECTING 2 LINES
----------------------------------------------------------------------
   8 | WARNING | [x] Unused use statement
     |         |     (Drupal.Classes.UnusedUseStatement.UnusedUse)
 200 | ERROR   | [x] Inline comments must end in full-stops, exclamation
     |         |     marks, colons, question marks, or closing
     |         |     parentheses
     |         |     (Drupal.Commenting.InlineComment.InvalidEndChar)

@@ -34,6 +36,7 @@ public static function getSubscribedEvents() {
'commerce_order.validate.pre_transition' => ['onValidateTransition', -100],
'commerce_order.fulfill.pre_transition' => ['onFulfillTransition', -100],
'commerce_order.cancel.pre_transition' => ['onCancelTransition', -100],
OrderEvents::ORDER_ASSIGN => ['onOrderAssign', -100],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't use constants above, so let's not use it here either, to stay consistent.
Core generally discourages constants and recommends the real IDs to be used.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't above because they don't exist. And that seems silly if core recommends real IDs yet constants provided. I'll change to keep it consistent here.

* Tests that an order assignment log is generated.
*/
public function testOrderAssignedLog() {
// Reassignment does not work on users
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment is confusing (doesn't work - why?). I suggest "Reassignment is currently only done on user login."

@bojanz bojanz force-pushed the 8.x-2.x branch 5 times, most recently from 1028fc1 to 005e904 Compare April 21, 2017 19:23
@mglaman mglaman force-pushed the 2861664-order-assignment-event branch from d0f4e89 to c060cf9 Compare April 23, 2017 09:42
@bojanz bojanz merged commit f226bfa into drupalcommerce:8.x-2.x Apr 25, 2017
bradjones1 pushed a commit to bradjones1/commerce that referenced this pull request May 4, 2017
nikathone pushed a commit to nikathone/commerce that referenced this pull request Feb 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants