Add geo-based currency determination

Created on 4 September 2024, 3 months ago
Updated 5 September 2024, 3 months ago

Problem/Motivation

We need to resolve currencies based on user location, through which I found https://www.drupal.org/project/commerce_currency_resolver β†’ . It seems like this module may be duplicating what's happening over there, aside from the commerce_currencies_price field type (which is much better UX in my opinion, so nice work there).

I'm able to get the two modules to work together for a simple order by just commenting out the services in this module and adding some handling for the currencies field in their CommerceCurrencyResolver.

Any thoughts? Just seems like a shame to have efforts duplicated across modules.

🌱 Plan
Status

Active

Version

1.0

Component

Code

Created by

πŸ‡ΊπŸ‡ΈUnited States mrweiner

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @mrweiner
  • πŸ‡­πŸ‡ΊHungary djg_tram

    This module clearly states in the README that it doesn't do any resolving on its own but uses resolvers found in the system. Which is, in my opinion, the correct approach because it separates the actual multi-currency support from a selection of additional behaviors whose requirements might be different from site to site. I was well aware of that module before I started working on mine and, frankly, this was one of the reasons, I needed a cleaner solution that makes the multi-currency handling as automatic as possible β€” and I thought that others might benefit from it, too.

    I added explicit support for Commerce Exchanger because it's a very common requirement (I use it, too). Other resolvers can be added in a similar way if it's not possible to rely on them automatically, without any extra support, just as part of the resolver chain.

  • πŸ‡ΊπŸ‡ΈUnited States mrweiner

    This was a poorly thought out issue for me to have opened. I think I see what you're saying. From an implementation perspective, I don't think there is an "integration" to be built here -- commerce_currency_resolver would just need to add support for your multi-currency field in their resolver for the two to be compatible.

    I think I'm just stuck in the middle where I like the approach and field over here, but I need the more robust geo-handling that they have over there.

    Their geo-based currency determination actually isn't in a resolver, but just some service and config handling in their CurrentCurrency service. So that might turn this into a feature request ticket for geo-handling, which would basically be copy-pasting their code into this module. That just results in the same code essentially being maintained in two places, though, which isn't ideal but maybe unavoidable.

  • πŸ‡ΊπŸ‡ΈUnited States mrweiner

    Or, I suppose, another option would be some way to leverage their CurrentCurrency service logic inside of commerce_currencies' CurrentCurrency logic, and then somehow bypass their resolver. Maybe this is just something I need to patch in on our end and not something you need to support.

  • πŸ‡­πŸ‡ΊHungary djg_tram

    This was a poorly thought out issue for me to have opened.

    Not necessarily. :-) Now that I looked into the code, it's not really much. It looks for either of the two dependent geo-ip packages and if present, asks them for the country. Really nothing duplicated, just a single call in each case. And then commerceguys/intl seems to maintain a country > currency map that could be queried to get the currency. All that could be placed into a very simple submodule providing this optional resolver. I'm not sure I'll have time to play with this for a week or two, and I also have something else I promised in another issue to look into but if you feel like experimenting with it, sure. Or wait a little and I'll try to do it a bit later.

  • πŸ‡ΊπŸ‡ΈUnited States mrweiner

    Updating the issue title :)

    Yeah you're right, simplest option would be to use that commerceguys/intl currency map. Commerce Currency Resolver has a whole admin config form to be able to select which currencies apply to which countries. But that's probably overkill in practice, and overrides could be handled with a hook/event. Good call.

    Yeah I'll do some experimentation. Do you think that an additional resolver is even needed for this? Seems like it could be handled in CurrenCurrency as an additional case. That's how CCR does it. There's a check for the modules in their config form, and then if the geo option is chosen then it's used in CurrentCurrency.

  • πŸ‡­πŸ‡ΊHungary djg_tram

    Yes, that's the possibility but then you probably would want a new setting for the field? Even if the geo module is present, the admin might not want to use it or not for all price fields.

  • πŸ‡ΊπŸ‡ΈUnited States mrweiner

    Yup, that's what I'm thinking.

  • Merge request !1Draft: Commerce currencies 3471924 β†’ (Open) created by mrweiner
  • πŸ‡ΊπŸ‡ΈUnited States mrweiner

    Alright, draft MR is ready that adds the basic geolocation functionality. Leaving it a draft until I can add interfaces for method documentation.

    • Adds configuration form/route + settings config and associated permission
    • Adds CurrencyHelper service to determine available geo modules, get currency for the user's country, etc
    • Updates CurrentCurrency to leverage geolocation if the setting is enabled
    • Updates the cookie case to leverage new isUsableCurrency method instead of using loadByProperties. This will be slightly more performant since loadByProperties is not cached, whereas loadMultiple is.

    I've added the geolocation setting as a module-level setting as opposed to a field-level setting. I'm not sure of a case where you'd want to use geolocation for one field's currency but not another. It would also add some complications due to interactions between field-level handling and the current global cookie handling.

Production build 0.71.5 2024