The order module provides no test coverage for TimestampEventSubscriber, to assert that the Placed timestamp is properly generated.

CommentFileSizeAuthor
#7 2848183-7.patch4.43 KBmglaman
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mglaman created an issue. See original summary.

agoradesign’s picture

agoradesign CreditAttribution: agoradesign commented

And I see, that the subscriber is using the REQUEST_TIME constant instead of the Time service -> that should be refactored when writing the test

vasike’s picture

vasike
Location Ramnicu Valcea
CreditAttribution: vasike at Centarro, Actualys commented
Status: Active » Needs review

And there is a PR for this : https://github.com/drupalcommerce/commerce/pull/624

p.s. used Commerce Time service for REQUEST_TIME value

agoradesign’s picture

agoradesign CreditAttribution: agoradesign commented

You should inject the service rather than calling \Drupal::service('commerce.time')

mglaman’s picture

mglaman
Primary language English
Location WI, USA
CreditAttribution: mglaman at Centarro commented
Status: Needs review » Needs work

Commented on the PR

vasike’s picture

vasike
Location Ramnicu Valcea
CreditAttribution: vasike at Centarro, Actualys commented
Status: Needs work » Needs review

PR updated.
Thank you : @agoradesign & @mglaman

mglaman’s picture

mglaman
Primary language English
Location WI, USA
CreditAttribution: mglaman at Centarro commented
FileSize
2848183-7.patch4.43 KB

Rebased the PR and here is the latest patch.

  • mglaman committed 631ee7d on 8.x-2.x authored by vasike 
    Issue #2848183 by mglaman, agoradesign, vasike: TimestampEventSubscriber...
mglaman’s picture

mglaman
Primary language English
Location WI, USA
CreditAttribution: mglaman at Centarro commented
Status: Needs review » Fixed

Thanks! Committed.

Status: Fixed » Closed (fixed)

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