🇺🇸United States @rhovland

rhovland → created an issue.

Feature all done. Ready for review

Update hook written.

The update fills in the variables and resaves the comment. Once that is done it removes the variables field from the database. I have to read the variables directly from the database because the field no longer exists in the entity.

I tested this update hook on a copy of our production database and examined the table contents before and after and all comments were correctly updated.

Please verify if I have done the translation part in the event subscriber correctly. I used commerce core as a guide.

Comment now shows up on the admin form.

Going to write an update hook next.

If comments on transactions could happen at a separate time from when the transaction was being created I could see it being apart from the entity. But it is part of the creation of the transaction and it makes sense there. Think of it like a comment on a payment (not a payment method) where it's a snapshot in time of something that happened, with additional relevant information attached to it.

If we added comments for gift cards then it would be inappropriate to put it on the giftcard entity.

I'm going to try to tackle this issue and get some code put together for review.

The comments field is definitely useful. Think of it like how commerce_log works. It keeps a list of transactions with details about what happened. The log can be what order a gift card was used on. It can be a gift card was purchased.

What is missing is when a transaction is added by an admin using the "add transaction" button, the comment is blank. That's not good. The admin should be able to enter a comment about the transaction they were creating.

The transaction entity actually already stores the order id associated with the transaction in reference_id and reference_type so an additional field would be redundant.

I think we can safely drop the variables field and keep what is already being recorded in the comments but doing it the correct way like this:

'comment' => $this->t('Used on order with id @order_id.', ['@order_id' => $order->id()]),

'comment' => $this->t('Bought @product.' ['@product' => $purchased_entity->label()]),

I did the translation at the entity creation instead of inside getComment() because that was the correct thing to do for a transaction comment. The variables thing is very weird and I did not understand why it was there so I didn't mess with it.

The changes should have actually been:

  public function getComment() {
    return $this->get('comment')->value;
  }

  /**
   * Registers gift card usage when the order is placed.
   *
   * @param \Drupal\commerce_order\Entity\OrderInterface $order
   *   The order.
   */
  protected function registerUsage(OrderInterface $order) {
    $adjustments = $order->collectAdjustments();
    foreach ($adjustments as $adjustment) {
      if ($adjustment->getType() != 'commerce_giftcard' || !$adjustment->getSourceId() || $adjustment->getAmount()->isZero()) {
        continue;
      }

      $giftcard = $this->entityTypeManager->getStorage('commerce_giftcard')->load($adjustment->getSourceId());

      // Create a transaction for each used gift card.
      if ($giftcard instanceof GiftcardInterface) {

        /** @var \Drupal\commerce_giftcard\Entity\GiftcardTransactionInterface $transaction */
        $transaction = $this->entityTypeManager->getStorage('commerce_giftcard_transaction')->create([
          'giftcard' => $giftcard->id(),
          'amount' => $adjustment->getAmount(),
          'reference_type' => $order->getEntityTypeId(),
          'reference_id' => $order->id(),
          'comment' => $this->t('Used on order with id @order_id.', ['@order_id' => $order->id()]),
        ]);
        $transaction->save();
      }
    }
  }

So get comment... gets the comment field contents.
And when the transaction is saved the comment is a translatable string using variable substitution. Instead of it being assembled later.

In light of this I think this issue should become
"Create completion pane to display purchased gift cards"

Because right now if a gift card is purchased the customer does not see the code. And shoving it into the variables for the completion message is wrong because now we'd have to override the template for that to fully implement the feature. It should be a separate pane that's added to the complete step of checkout.

As part of this feature we can figure out a better way to handle the query to get the list of gift cards that were purchased.

Ok I think I figured out where the source of the confusion is here.

I thought that the checkout completion message variables were intended to display the gift card codes redeemed to pay for the order (just like how payment methods are usable variables). This code was actually intended to allow to display the codes for purchased gift cards. It does this by querying gift card transactions that have a reference_id that matches the item id. However this is a very big problem as reference_id is shared by two different types of entity ids!

OrderEventSubscriber::giftcardPurchase() generates gift card codes for gift cards that were purchased as an order item. It records the order item id in the reference_id field in commerce_giftcard_transaction.

OrderEventSubscriber::registerUsage() generates a transaction whenever a gift card is used to pay for an order. It records the order id in the reference_id field in commerce_giftcard_transaction.

We're storing two different ids in the same field with no way to differentiate the difference between transaction types other than the content of the comment field.

Yeah the patch I wrote was a rough idea that needed more work. I did not have the skill to do much more than what's in it.

Missed a line of code where $rating_element was being set. Link now properly generates.

Still wouldn't explain your problem of stars not showing up since the old code was still creating the rating element. Think it's probably the javascript library missing.

Apologies I had opened this issue to push my code and fell asleep before I finished. I pushed them in the morning. The actual feature this issue is for is done. The only thing remaining to do was the update hook so that existing site configuration is migrated over to the new structure. Also I had not made any changes to the configuration schema file yet which needs to be updated to reflect the changes in configuration.

If the stars aren't displaying then that means the rateit library isn't loading. It's displaying like this on my dev site

rhovland → created an issue.

There was several situations where the add review button should have been displayed but wasn't, or was displayed but shouldn't.

I moved the review type and already reviewed check out of reviewForm() into accessReviewForm() so the "Add your own review" link only shows up when all access checks pass.

I also in a separate commit allowed anonymous users to access the form, where they will be redirected to login so the add link appears to them. This is to invite users to login and leave a review when on the product's review page. The redirect could only be done on the form due to limitations on how drupal handles route access.

Feature done. Needs the schema updated and an update hook written to migrate setting "show_overview_link" to "link_display" = "overview"

rhovland → created an issue.

Ready to go

rhovland → created an issue.

getBalance() effectively cannot return null due to several constraints:

The balance field is defined as required in the Giftcard entity:

$fields['balance'] = BaseFieldDefinition::create('commerce_price')
  ->setLabel(t('Balance'))
  ->setDescription(t('Current balance'))
  ->setRequired(TRUE)

Entity validation enforces this. In GiftcardTest::testSaving() an entity without a balance fails validation.

All code paths that create gift cards set a balance:

• Direct creation sets a balance
• Gift card purchases set a balance
• Bulk generation sets a balance

The Redemption pane has a check for null when it probably should be checking for zero

$balance = $giftcard->getBalance();
if (!$balance) {
  $form_state->setError($pane_form, $this->t('Unable to check gift card %code balance, please try again.', ['%code' => $giftcard->getCode()]));
}

Becomes

if ($giftcard->getBalance()->isZero()) {
  $form_state->setError($pane_form, $this->t('The provided gift card %code has no balance.', ['%code' => $giftcard->getCode()]));
}

Created a readme file in this issue 📌 Create a readme file Active which includes instructions for creating a purchasable gift card. The instructions are also now on the project's page → so I'm closing this as fixed.

Initial work done. I also copied this to the module's front page to replace the sparse presentation currently there.

Please review for language and clarity and let me know if this is good to commit.

rhovland → created an issue.

rhovland → created an issue.

So while I was looking at the history for this issue to revert the changes made, I ran into this issue's bug. I viewed a comparison, then hit back and then tried to view the comparison again and the button didn't work until I reloaded the page.

Why did you overwrite all of the changes made to this issue?

There is already an issue for this.
📌 Drupal 11 compatibility Active

Drupal 11 is a requirement for commerce 3.x. Drupal 11 needs to be added to the composer.json. This module has depreciations that need to be fixed for it to be Drupal 11 compatible.

I attempted to address the issue summary. Can you please let me know if there needs to be further changes to it.

In regards to a test, where do you suggest it be placed? It seems FunctionalJavascript tests for forms are located at core/tests/Drupal/FunctionalJavascriptTests/Core/Form. Should a new test file be created there? Should it be added to an existing Form test?

When writing the test what am I testing for? Am I writing a test to ensure that the submit button is disabled when clicked twice? And then navigate back and ensure that it is not disabled? I was unable to find an existing test for the double click prevention feature.

Lastly, is there a way to ensure someone with more experience with javascript reviews the merge request for correctness? I'm uncertain if I did it correctly. There's also a failure of a nightwatch test that I don't really understand (I've never used nightwatch before).

Appreciate your feedback, I'm still learning and have only done work with tests in contrib modules.

Fixed UPSSdk::getShipmentShopRates so it returns the expected array structure when there is only one rate.

rhovland → created an issue.

I have submitted a MR to fix this upstream
https://www.drupal.org/project/drupal/issues/3144382#comment-16037225 🐛 Prevention of multiple submits blocks use of back navigation Active

I wrote an event listener for form.js that unsets the double submit data if the page is loaded via the cache (user hits back button), allowing the submit button to work as expected.
If core wants to prevent ever resubmitting a form then it should be a configurable option not a blanket form API feature.

As written this prevents the problem from the original issue, clicking submit multiple times, while avoiding breaking forms when the back/forward buttons are used.

In my testing it affects both Firefox and Chromium.

This affects all submit buttons in a form not just the one that was previously clicked.

So for an example I have a cart form. It has multiple submit buttons. The main one is the checkout button. There are also remove buttons on each cart item and an empty cart button. None of these buttons are functional after clicking back in the browser after submitting the form.

This is the issue where this feature was added
🐛 Double click prevention on form submission Fixed

The feature was intended to prevent double clicking a submit button causing two form submits from happening. It was not intended to prevent the same form from being submitted again after loading the submission page and going back in the browser.

This feature as written causes unexpected behavior in forms across core and contrib. There is no mechanism to opt out a form from this protection. The only way for an end user to fix the page is to refresh the page or change a form value (if they can).

There is a Drupal 10/11 compatible module here https://www.drupal.org/project/google_customer_reviews →

There is a Drupal 10/11 compatible version here https://www.drupal.org/project/google_customer_reviews →

rhovland → created an issue.

rhovland → created an issue.

rhovland → created an issue.

For more information about the changes in ba662d58 see the following for more information:
Avoid using constructors in extended plugin classes:
https://www.previousnext.com.au/blog/safely-extending-drupal-10-plugin-c...
https://www.drupal.org/node/3468511 →
https://www.drupal.org/node/3076421 →

Assigning entity storage to a property instead of the entity manager:
https://mglaman.dev/blog/dependency-injection-anti-patterns-drupal

I'm going to put some time into evaluating janephp and openapi-generator and create a library for USPS using one of these generators. Using a generator will also help with adapting to subsequent changes to the API as we can just feed the new spec into it and audit the changes made for accuracy.

Once the library is made I'll create a new version of the USPS module and have the maintainers merge that into this project.

This is going to take a few months as I will be working on this in the spare time between projects at work. But now that there's an end date the urgency of this has placed this higher on my list.

Ok looks like we now have a end of service date
"The Web Tools API platform will be retired on January 25, 2026."
https://www.usps.com/business/web-tools-apis/welcome.htm

The first thing that needs to happen is a PHP API client being made

Once there is a client (the script I linked is not sufficient) then a new major version of this module can be created and adjusted to use the API client.

That depreciation is handled in 📌 Implement validation tests Active

Changed the above listed language items in the form.
Added a semicolon to the form title so it's now "Review product: Product Name"

Thoughts on changes? Anything I should consider tweaking?

Compatibility fixed. Ready for review.

rhovland → created an issue.

All tests pass now

Fixes for test failures added

rhovland → created an issue.

rhovland → created an issue.

Made some changes to the readme file for the configuration section to hopefully make it clearer how to setup the module.

rhovland → made their first commit to this issue’s fork.

Test finished for 🐛 WSOD on order completion page when order has items without purchased entites Active

Created a test which will pass once 🐛 Fix tests Active is merged into this issue branch.

https://git.drupalcode.org/issue/commerce_giftcard-3481607/-/pipelines/4...

I was unaware there was a project owner I thought all maintainers were effectively owners of the project.
In light of this it seems I was actually intending to become project owner but I did not know there was a difference between the terms.

So @prodigia I would like to become project owner and keep you as a maintainer. Do you accept this request?

Agreed but didn't change anything? https://www.drupal.org/project/google_customer_reviews →

Confirmed to fill the array with redeemed gift card codes. Needs a test to make sure the codes are output into the template variables. Since there is no template provided to display these values we probably can stop at making sure the template variables are populated in a test.

Code cleanup done. This is ready for review and commit and so is 🐛 WSOD on order completion page when order has items without purchased entites Active

Created a separate branch with the code changes from 🐛 WSOD on order completion page when order has items without purchased entites Active .

All tests now pass once both issues are committed.

Doing some code cleanup on tests and then this is ready for review.

rhovland → created an issue.

Fixed tests.
I removed the taxes from the test because they were breaking things and as implemented wasn't actually testing the interaction between taxes and giftcards. I opened a new issue to add a separate test for taxes. 📌 Add a test for tax interaction Active

Form submissions (review step, complete step) were broken because of upstream commerce changes. I used the CouponRedemptionPaneTest in upstream as a basis for changes to the test setup. This fixed form submissions.

There are 3 remaining test failures. They are legitimate failures caused by faulty code in commerce_giftcard.module. Specifically commerce_giftcard_preprocess_commerce_checkout_completion_message assumes that order items have purchased entities attached to them. This is optional for order items and will cause a WSOD on the checkout complete step of any stores creating basic order items on orders with no purchased entities attached (probably not common).
Additionally, the code wouldn't ever work anyways because it assumes that giftcard transactions have a reference to the order item id instead of the order id which is what is actually stored on transactions.

I will work to fix this bug tomorrow and button up the tests with some additional cleanup.

rhovland → created an issue.

CouponRedemptionPaneTest test passes. The test failure is from another unrelated test.

rhovland → created an issue.

Digging into the code of this test I'm starting to doubt it ever worked. It was literally in the wrong namespace (commerce_promotion) until the most recent changes to fix phpcs violations and such. Other than having taxes shoved in there there's a ton of similarities to commerce_promotion's CouponRedemptionPaneTest.

The problem as it stands is that I don't know how this custom made up tax is supposed to work so I can't begin to tell if the assert statements are testing it correctly and there's an issue with the tax setup or if the assertions are wrong and tax setup is correct.

I'll keep digging but I thought I'd share what I've found so far.

Ok I figured out what was causing the last group of totals errors. The creation of the tax is not working anymore in the setUp() function for the GiftcardRedemptionPaneTest. So the expected totals are off because the order has no tax on it.

What if instead of pre-filling it had a button next to it called "Generate code" that would fill in a code when clicked.

Waiting for a reply

rhovland → made their first commit to this issue’s fork.

Ok I think I did credit correctly please double check.

Ha I actually fixed the testRefund test...

Looking at the tests in commerce I noticed absolutely none of them clicked on order transition links/buttons and instead triggered a state change in the order with code then saved it. Clicking the place order button wasn't changing the order's state from draft. Additionally for whatever reason the refund gift card action link wasn't appearing on the resulting page.

Since the important parts of the first section of the test are verifying that the order has been placed (in completed state and gift card balance changed) and that the refund route is accessible I switched to invoking those directly. These should be functionally the same for the purposes of the test. Once on the refund page the rest of the test can proceed normally.

Working on moving the needed changes over to the merge request branch

rhovland → created an issue.

I'd love to but I'm not exactly sure what "adjust the credits" means

Remaining errors:
GiftcardAdminTest::testRefund The text "Refund gift card" was not found anywhere in the text of the current page.

Best I can tell "Refund gift card" is not on the page after placing an order because the current user in the test doesn't have permissions for that route.

GiftcardRedemptionPaneTest:

All of these errors don't make sense because they should be present. The output of these tests aren't in the artifacts so I can't tell what is wrong here.

This last commit to fix getComment() brings the phpcs error "Only string literals should be passed to t() where possible"

From what I can understand passing variables to t() is wrong and should not be done. The source of the comments is where translation is supposed to be happening. Only done in the event subscriber.

rhovland → made their first commit to this issue’s fork.

Oh also I need to know if you are interested in remaining as a maintainer of this project

Thank you. To finish this and close the issue I need you to add me as a maintainer.

After waiting for google to update our merchant center data I can confirm the module I wrote for this is functioning and is ready for release.

Tested in a site install. Works as expected. Waiting for input on language changes then will commit

Looks good, I just need to test it.

How do you feel about the language changes I made to the configuration form?

rhovland → made their first commit to this issue’s fork.

rhovland → created an issue.

I need some more clarification here. Are you arguing that details about the payment method should not be stored on the payment? Or that a more robust receipt storage is missing in commerce?

There currently is no receipt entity in commerce. There's the order which is also treated as the invoice or maybe receipt of what was agreed upon. The payment is attached to the order. That records information about how it was paid for. Usually a third payment gateway stores a transaction on their end with information about the payment made. If it's an internal payment gateway (such as manual payment for example) then the payment entity itself is the receipt of payment. The payment is designed to be the record either directly or in reference to an external record.

The goal of this issue is to store more information about the payment transaction on the payment itself (easy) instead of having to retrieve the information from the 3rd party payment processor (complicated).

In commerce, the payment is the receipt. It's the record of the payment. The payment method on the other hand is something that changes over time.
Typically on a receipt (payment) you want a record of what payment method was used. Currently commerce does this with a reference to the payment method that was used.
Since the payment method is not static and can change at any moment after the payment is recorded the information about the payment method can be lost.
This issue aims to add information about the payment method to the receipt (payment) so there is a record of it at the time of payment.

So I got in contact with the API team and they told me that state code is required for US, CA, PR and not required for other countries. The field can just be left blank if it's not one of the countries that require it.

There doesn't seem to be a problem truncating codes to 2 characters as a fallback

$fedex_rest_address->setStateOrProvince($address->getAdministrativeArea())

to

$fedex_rest_address->setStateOrProvince(substr($address->getAdministrativeArea(),0,2))