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))
Still cannot find a list of two letter codes anywhere. The ISO documents are all 2-3 letter codes. Fedex publishes some but not all at https://developer.fedex.com/api/en-us/guides/api-reference.html#canadapr...
Tests pass. Ready for review and commit.
rhovland → changed the visibility of the branch 3494895-argument-1-stateorprovince to active.
rhovland → changed the visibility of the branch 3494895-argument-1-stateorprovince to hidden.
rhovland → created an issue.
rhovland → created an issue.
The goal of this was to get the feature added instead of a whole address validation system implemented. The current state of the other issue is far from complete and it's not even clear how address validation should be implemented.
If the goal of the other issue is to implement residential address checking then it should be updated to reflect that.
Thank you for making these updates. I tested the 0eecdb17 version of the MR and it works!
This also affects 2.x but the REST API responds with a much more useful error message:
Total package count cannot exceed the limit of {PACKAGE_COUNT} packages.
Examining the request sent I see:
'groupPackageCount' => 98891
There is only one package. Examining $requested_package_line_item->sequenceNumber the value is 1
The group package count is set using $requested_package_line_item->setGroupPackageCount($count) and $count is calculated with static::calculatePackageCount($requested_package_line_item, $packages[$requested_package_line_item->sequenceNumber - 1]) which gives a result of 98891.
This is a result of the default 1x1x1mm package being selected.
If the configured box is smaller than the items going into it this math will work out a huge number of packages despite it being an impossible shipment.
If we don't want to introduce anymore complexity into the Calculate option then at least the description needs to be changed to something like:
"Calculate" will take the volume of the package type selected in this shipping method and divide it by the total product volume of the order. Fedex will return a single rate for the number of packages with the selected dimensions and weight evenly divided between them.
I think I probably should add a configuration option to the shipping method that allows switching this on and off because this does add a small delay to uncached address queries. Maybe an option to mark all packages as residential too, skipping the API check.
rhovland → created an issue.
rhovland → created an issue.
rhovland → created an issue.
I investigated the source of $shipment_title->render() and did not find an issue that it was added to solve. It seems to be vestige of an earlier module version and I can't find a reason why it would be needed.
Issue fixed. Uses null coalescing operator to set a package name if $shipment_title is null.
I moved $shipment_title = $shipment->getTitle() and $package_type = $shipment->getPackageType() outside of the foreach loop because the loop is for packages not shipments and those variables don't need to be reset for every package because they will never change.
rhovland → created an issue.
rhovland → created an issue. See original summary → .
Looks like this affects other users as well but only some of them.
rhovland → created an issue.
Made the changes to the label and description of the password field. Do we want to change the machine name of the api_pass field or keep it as it is?
I also clarified the language in the account number description to account for the fact that the API key is tied to a specific account number which may be different than the main account number (such as test api credentials).
All tests passing now
rhovland → created an issue.
rhovland → made their first commit to this issue’s fork.
The StringTranslationTrait is an essential component to allow for translations into other languages. Removing it is not an acceptable fix.
I am unable to reproduce this error. Can you provide more information about how to reproduce it? Are you using current versions of Drupal, commerce, and commerce shipping?
Test failures are unrelated to changes
MR fixes issue by generating css variables and attaching them to :root. Comparing the variables from styles.css and ck5styles.css reveals an identical output. The rest of ck5styles.css is unchanged.
rhovland → created an issue.
Can confirm this fixes the issue in Drupal 10.3.9. While I wasn't seeing any performance impacts in most areas of my site the permissions page was completely broken and hit the server timeout whenever I tried to load it. Applying this MR as a patch restores the performance.
When I tried it it masked the entire menu item. Maybe I did it wrong? I'm not familiar with using mask to change svg color.
I just tested it again. This time using the standard profile. Enabled the media module, added the embed media button to the basic text format.
Created a new basic page. Clicked the add media button. Class media-library-widget-modal is missing.
rhovland → created an issue.
Was able to reuse the css variables for the icons and use a css filter to set their color to match the administration toolbar.
I added styles for the toolbar-tray. I did not add styles for the toolbar-bar since an admin cannot move menu items into that menu.
I was not able to use mask to set the color because that requires an actual dom element to function properly and modifying the structure of the toolbar isn't really a good idea if it's even possible.
rhovland → created an issue.
This was fixed in
📌
Unify 'Giftcard' and 'Giftcards' Terminology Across the Codebase
Active
Providing a default value following the rules for the giftcard type seems pretty straight forward to me. If someone wants to enter a custom code they replace the default value in the text box.
If this was a setting, where would it be configured?
Normally I'd agree except in the vast majority of cases there will be only one gift card redeemed (if multiple is even enabled on the site). Additionally using a list will introduce usually undesirable visual changes that would then have to be themed away by site builders. While the module could provide some css to hide the list bullets, now we'd have to maintain CSS in the project where there was none before.
MR obviously needs to be changed if ✨ Gift card theme uses a table to display redeemed gift cards Active is merged.
Also I wrote this a really long time ago and I don't really understand what that code block in the inline form is doing so I still need to do some research to make sure it's correct.
rhovland → created an issue.
Closing this in favor of followup issues.
✨
Display the gift card balance on the redemption form
Active
📌
Checkout pane language cleanup
Active
✨
Gift card pane display only looks correct in the sidebar
Active
rhovland → created an issue.
rhovland → created an issue.
Looks like this is a problem with my site's theme instead of this module. Closing.
Good to go. Tests pass and I have verified this fixes the problem.
While this probably isn't a super common situation to run into it's possible if someone installs commerce and commerce_giftcard and then runs drush pm:enable commerce commerce_giftcard without explicitly enabling any of the other commerce submodules.
This results in a broken database where you can't install commerce_store or remove commerce_giftcard to fix the situation.
Tested with the change in the MR. Now it installs correctly.
rhovland → created an issue.
rhovland → created an issue.
Tested MR. Everything works as expected.
Is there anything else that needs addressed in this issue or is this good to go.
Tested MR and can confirm it works. Seems ready to commit.
rhovland → created an issue.
rhovland → created an issue.
This module implements the bootstrap 5 built-in carousel, which looks like this https://getbootstrap.com/docs/5.3/components/carousel/#indicators
The styling of the indicators is different.
As for the image sizing problem can you show us what result you're getting?
For some reason the MR removed the required attribute for the code field on the giftcard entity. This has been reversed.
Also fixed a phpcs error.
So I started making a patch for this and stopped realizing that how this is implemented is probably entirely wrong. Most panes are composed of fieldsets with titles that are generated by drupal. Meanwhile this pane uses a theme template to display the pane title which will not match the other panes if the site theme is different than the default. For example, my patch uses an H5 tag, but the other one is a H3. Which one is correct? Most panes don't even have templates. They just create a form using the API.
I will spend some more time evaluating how other panes are built and build a new MR that addresses these concerns.
Did you file this issue against the wrong version of the module? 5.5.x is for the Bootstrap 5 module compatible with Drupal 11.
rhovland → created an issue.
In my testing the carousel image scales with the viewport. Are you referring to something else when you say responsive?
The default is to hide the description at the medium breakpoint and below. You need to change your settings for the carousel. Specifically the "hide captions" settings. And make sure "Add captions to your slides for add title and description over the image" is checked.
"position-relative" is necessary for the captions (absolute position) to align to the carousel item. Normally this is the element with "carousel-inner". But because this module allows for doing grid layouts inside the carousel the "col" elements are lacking this anchor for the captions which are absolute positioned.
Before this change any absolute positioned element within a carousel (eg the caption) would be positioned according the first parent element with a relative position property. That was the carousel container instead of the carousel item.
This bug fix essentially changed the parent element your absolutely positioned image aligns to. Why does your image have an absolute position?
After discussing this with the maintainer on Slack, they would like to migrate to using Webpack for this module and others as they did in another one of their modules
https://git.drupalcode.org/project/varbase_layout_builder/-/blob/10.1.x/...
Tests passing. Errors unrelated to MR.
rhovland → created an issue.
Comparing template changes between the 3.x and 4.x branch is as easy as cloning the module's git repo and then running the following in the cloned folder:
git diff 3.0.x 4.0.x -- templates/
So far only changes to content/media.html.twig
Further information is required. This is functioning normally on my test instance of commerce. Is it possible you have an issue with your site configuration/templates that is causing the email to disappear for anon customers?
Login method is outside the scope of commerce and as you discovered the email registration module provides an alternate login method that is compatible with commerce.
Closing this issue. If you feel this is in error please re-open.
This issue is a duplicate of ✨ Send Drupal Core welcome email to new registrations during checkout Needs work
When addressing spelling issues adding words to the dictionary should only happen for valid words.
For instance "fewfsfs" is part of an API key. This should not be added to the dictionary. Instead the the following should be appended to the line above the API key in the test
// cspell:disable-next-line
The address in the test "2630 Hegal Place" should be modified so it does not trip the spell checker.
The usage of "autofills" is incorrect. It should say "Chrome autofill incorrectly fills this field with address line 1" as that is clearer language.
The improvements in #5 were lost. #5 was already Drupal 10 compatible. #8 should not have been committed.
rhovland → created an issue.
rhovland → created an issue.
I'm on the Drupal slack under the same username. We use this module regularly, albeit a heavily modified branch of it that I maintain myself. The number of patches was getting to hard to manage. If you're in need of a co-maintainer I'd be happy to help. I was already in planning to apply for it after I finished up work on another module.