Closed (fixed)
Project:
Currency
Version:
8.x-3.x-dev
Component:
Code
Priority:
Major
Category:
Bug report
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
16 Jul 2014 at 22:27 UTC
Updated:
27 Apr 2021 at 08:41 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #1
xanoWhat about adding an Import currency local action next to Add new currency, and provide a selection form that lists all currencies. The user picks one, and the currency is immediately imported. The user can then either be redirected to the entity list, or to the edit form of the imported currency to make some possible final changes.
I haven't looked into this yet, but do you know how easy it is to import config into a full entity object? Is the config entity import code reusable enough?
Comment #2
berdirThe code I referenced does import config entities. So yes, something like that could work, or you could somehow combine it into a single form, similar to how language does it at admin/config/regional/language/add
You can chose between an existing ready-to-import currency or select "Custom currency".
Comment #3
xanoI don't think the way
admin/config/regional/language/adddoes it is good UX. You have to read the help text to understand that adding a custom currency requires you to select the last option from the list. I'd like the Currency UI to communicate the two possible actions more clearly than this.I hadn't followed the link yet. Thank you. It's exactly what we need.
Comment #4
xanoThe config import example code you linked to was very useful. Thanks!
This patch has a working solution (without test coverage as I approached this from the UI side first). It feels unambiguous, except maybe for the fact that we have two local actions and newcomers may not understand the difference straight away.
Comment #5
xanoIf we use this solution, we will also need to remove
currency_install().Comment #7
xanoThe tests fail (mostly) because they expect currencies to be imported by default.
Comment #8
xanoThis patch does the same thing for currency locales, as there are a couple hundred of them as well. It also moved the import code to a dedicated service. It still needs test coverage.
Comment #11
xanoI'm considering moving all currency metadata back to bartfeenstra/currency again after this, so the data can be maintained separately. This was done for Currency 7.x-2.x and the only reason this wasn't done for 8.x-3.x, is that I wanted default config to be imported on install.
Comment #13
berdirHaven' tested yet, but setting to major as I think this is pretty important. My config export folder exploded with adding currency, makes it harder to work with that. And also slower, but didn't profile/benchmark this (yet)
Comment #16
berdirFinally have some numbers. On a somewhat slow, virtual system, installing currency took 2.4 minutes.
Most other modules take 4-5 seconds on the same system (comparing timings of batch requests in the installer).
So yes, we need this. Should also speed up web tests nicely ;)
Comment #19
xanoComment #20
xanoComment #22
xano- Added test coverage for the new code.
- Moved all default config to the same directory. This allows the code to be much simpler.
- The XXX currency is now imported upon installation, because it is the non-removable default currency.
- Shorthand array syntax.
Comment #24
xanoComment #25
xanoAnd now including file moves instead of deletions.
Comment #28
xanoComment #29
berdirYes, as expected, currency install went down to the usual 4-5seconds on the system that I tested. Which is a virtual service with pretty slow IO. The difference is a lot smaller on my laptop with SSD.
Didn't do a code review, but +1 in general on the performance aspect.
Comment #30
xanoThanks!
Comment #32
xanoLet's fail gracefully (and in accordance with the interface) when importing the same entities twice.
Comment #34
xanoComment #36
nnevillIssue has been closed 7 years ago :)
But I still have what to add here. I've attached small patch which allow easily import all currencies after module installation. I don't think it makes sense to import them during module installation because probably you don't need all of them. But the ability to import several currencies at once makes much sense for me.
So to import currencies go to /admin/config/regional/currency, click "Import a currency", select currencies you have to import and click "Import".
Hope this small patch will be useful for someone else.