🇳🇴Norway @zaporylie

Moving to the Project Ownership issue queue

Hi, I would like to adopt this module and help bringing it up to date.

zaporylie → created an issue.

This is a great addition and I am all for it. We need a simple test coverage to make sure no regressions are added in the future.

There are some errors to be addressed as a result of enabling Gitlab CI but IMHO that shouldn't block the addition of Gitlab CI to be added. Some will be trivial to fix, but some may need to be addressed in a way that changes how the module operates.

Well, this module is not dependent on a fixed version of Commerce Core so closing it. Please consider merging in 📌 Enable Gitlab CI Active to ensure the compatibility.

zaporylie → created an issue.

zaporylie → created an issue.

zaporylie → created an issue.

I updated the issue summary a bit to better reflect the issue. MR looks good—I tested it locally, and the problem of the password being reset on every form edit submission has gone away.

I set the severity to Critical as in the current state the module doesn't work.

zaporylie → created an issue.

This is critical issue as the sheet configuration cannot be set without this patch.

What is the main purpose of this module other than being a wrapper over a composer package that can be added directly? I am a bit confused tbh

zaporylie → created an issue.

Here's an example recipe.yml file I've been using along with the proposed MR to define a currency by simply applying the recipe and providing the required input.

name: 'Commerce Kickstart Currency'
description: 'Allows importing user-selected currency'
type: 'Commerce Kickstart'
install:
  - commerce_price
input:
  currency:
    data_type: string
    description: 'Main currency. You can add other currencies later'
    constraints:
      # todo: Add currency constraint.
      NotBlank: []
    prompt:
      method: ask
      arguments:
        question: "Enter the currency you want to install on your site. The currency must be in 3-letter format, ex. USD, EUR, or PLN"
    form:
      '#type': 'select'
      '#title': 'Currency'
      '#description': "Enter the currency you want to install on your site. The currency must be in 3-letter format, ex. USD, EUR, or PLN"
      # todo: Add options callback.
    default:
      source: value
      value: 'USD'

config:
  actions:
    commerce_price.commerce_currency.XYZ:
      currencyImport: ${currency}

zaporylie → created an issue.

It seems like my understanding was wrong :) We are still able to define a custom config action plugin (currencyImporter) that utilizes a currency importer service for dynamically defining currency config entity. The only non-clean part is the signature of the ConfigActionPluginInterface::apply(string $configName, mixed $value): void; that requires $configName which in my case would be something like commerce_price.commerce_currency.XYZ so it doesn't do much and feels hacky. Otherwise, it seems like I can achieve what I am aiming for.

Re:

I propose one hard limitation up front: inputs cannot be used for dynamic IDs (like "enter the name of your content editor role"). Why? Because that makes recipes unpredictable, which would in turn make them less composable, which would violate a foundational design goal. Recipes are automatons, not functions. Inputs can only be used in the parameters passed to config actions.

(Having just written all that, I could see a way to maybe pull off dynamic IDs without breaking composability, but it would need further thought and hashing out, and we could always add it later. It's definitely out of this issue's scope.)

Is there an issue where dynamic IDs as being actively discussed? For all Commerce projects, we'd need to require the currency input from users applying the recipe. Currencies are config entities and the currency code is featured in the config ID. My understanding is that creating a Currency entity programmatically based on the user input would not be possible with the current state of things until support for dynamic IDs is added which is effectively blocking Commerce-based recipes.

One more note - we will soon be tagging a new release of Drupal Commerce and while it is not critical to get this in ASAP it would be nice to use that opportunity to have the logo featured in a tagged release.

Added the logo with a purple background in 512x512 resolution. My understanding is that it is in line with the requirements for Project Browser but I couldn't find a canonical resource to back up my understanding.

zaporylie → changed the visibility of the branch 3372681-commerce-core-logo to hidden.

Merged. Thanks everyone involved.

Thanks @dpi for this. I see you also uncovered a bug with dependency injection for the config form. I added 📌 The settings form is missing test coverage Active to add a test coverage for future-proofing this part. Also changed how dependency injection is handled in your MR and going to merge it now.

zaporylie → created an issue.

The MR cannot be merged. Please rebase and ping me when ready.

I decided to mark D7 as no longer supported therefore closing this as won't fix. Sorry.

To leverage Gitlab CI I need an up-to-date MR that will make sure tests are green before this is merged in.

Well, you're referring to only part of my comment :) And if this is what should be needed then I still wouldn't call it a bug but a missing feature.

But there's also the other view on this module's usefulness (and highlighted risk) that I tried to point out in my previous comment - the one about taking an order snapshot. In such cases, by design, the reports are never to be re-generated because they keep a record of the state of order at a certain time, ie. order placement transition.

When you say

The reports have NEVER been generated without clicking "Generate reports" manually.

do you mean that in your case reports are never GENERATED or never REGENERATED upon the UPDATE? Because if for PLACED orders you still don't see reports UNTIL button in the interface is clicked then yes, there's a bug somewhere :)

For which of the existing reports would it make sense to have outdated reports?

All of them? :) I wouldn't call it an outdated thought as it is very much valid at the time of generation.

I guess it all depends on how we look at this feature (or sell this feature to our clients). If we think of each report type as a storage derivative for the order model (which is kinda how it's presented on this module's page ATM) and the generation of the report is only for the sake of keeping data in the more queryable format then it makes sense to regenerate every time order is saved/updated.

But if we look at it from a different angle and say that reports are snapshots of the order state in a particular moment in time (by design - order placement) then regeneration should never be required. In fact, just looking at some plugins, the regeneration will not work great if ex. promotion is deleted by the time of regeneration. or default tax rate changes/is deleted, etc.

Could you elaborate on why having such a function would be beneficial?

I kinda understand why would that be available via UI or perhaps CLI (for maintenance purposes) but, IMHO reports should be persistent and basically never regenerated as they should reflect the snapshot of the order state at a certain moment in time.

All other issues are blocked by this one as tests are currently failing against D11. I left a comment on the MR (requested feedback).

richo_au → credited zaporylie → .

I am the category to Task - it is not a bug as this functionality is neither implemented nor supported. Still, it is not a support request either because sometimes this cannot be "fixed" using the SameSite cookie.

I also changed the version to 3.0.x which is where this would have to go.

Seems like we're discussing the same problem but with 2 different origins here:
- redirect from the offsite gateway via POST - this is where SameSite cookie will help
- redirect from offsite gateway to default browser with no user-session as the checkout was initiated in the non-default browser, most commonly - in-app browser (facebook, Instagram, etc) - in that case, SameSite cookie won't help.

Please note that this MR is good to merge even if fails. The objective here is to enable tests which can be fixed in a separate issues, one by one.

zaporylie → created an issue.

I have relaxed the Commerce requirement and updated gitlab ci template.

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

zaporylie → created an issue.

zaporylie → created an issue.

zaporylie → created an issue.

zaporylie → created an issue. See original summary → .

I stumbled upon this issue when debugging not working migration plugins generated with drush gen migration. It turned out the generator was creating the plugin in the migration directory as opposed to migrations (singular vs plural).

I see many references on the internet referring to `migration` directory, and this issue is one of them. That creates some confusion so I had to debug the code to find out which directory exactly is to be used.

zaporylie → created an issue.

MR is ready for mergin. The only known missing feature at this point is that the list of available shipping methods is fixed and it doesn't depend on the address selected within Vipps MobilePay Checkout.

zaporylie → created an issue.

Adding @borgenk as he reported this issue.

zaporylie → created an issue.

Added credit for @borgenk as the reporter of this issue.

zaporylie → created an issue.

Adding @borgenk as he was the one reporting this to me.

zaporylie → created an issue.

Added credit for @borgenk as he reported this bug

zaporylie → created an issue.

I took pictures and contributed them back to Flickr

While I didn't specifically signed up for the photo team I took some photos and contributed them back to Flickr.

While I didn't specifically signed up for the photo team I took some photos and contributed them back to Flickr.

I took care of Room 3

I took care of Room 3

It was nice meeting you all. Thanks for this.

zaporylie → created an issue.

The module is already compatible with D11 and doesn't limit Commerce compatibility to 2.0 only so this issue can be closed.

Fixed thanks to 🐛 Module vipps_mobilepay_commerce recognized by composer as metapackage Fixed

Changing client

zaporylie → created an issue.

I would feel much better if we had any kind of testing included in the MR that would fail if we accidentally introduce regression in the future (change interface name, namespace, signature, etc). I believe https://git.drupalcode.org/project/commerce/-/blob/3.0.x/modules/product... is a good candidate where the interface can be checked. This way if we ever mess up with it, at least we have a asset somewhere in our test suite.

I am unsure if the AvailabilityOrderProcessor is the right place to dispatch the event. if it's dispatched from somewhere I think it should be the AvailabilityManager itself.

Frankly, I am not sold on the idea of removing unavailable order items from the cart either. I think this results in very bad UX, especially when an order item is deleted programmatically without the customer really noticing it. This could be a problem, especially if the average cart contains many order items with low prices and low quantities, like groceries, etc. In such case the customer might not even notice that items were removed.

In fact, all projects that I implemented in the recent past had the AvailabilityOrderProcessor removed altogether because of that non-optimal UX behavior.

I think a possibly better way to approach this is to label unavailable order items as such during AvailabilityOrderProcessor run ✨ If order item is not available prevent checkout completion rather than deleting it from cart Active