Problem/Motivation

Drupal\commerce_pos_currency_denominations\Form;

Proposed resolution

done

Remaining tasks

User interface changes

API changes

Data model changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

rakesh.gectcr created an issue. See original summary.

rakesh.gectcr’s picture

Parent issue: » #2903634: Alpha 1 - Meta
rakesh.gectcr’s picture

[Wed Aug 23 20:20:13.317053 2017] [php7:notice] [pid 12357] [client 127.0.0.1:51666] Uncaught PHP Exception Drupal\\Core\\Entity\\EntityMalformedException: "The entity does not have an ID." at /Users/rakeshjames/Sites/mystore/web/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php line 248, referer: http://mystore.lo/admin/commerce/config/currency_denominations/add

rakesh.gectcr’s picture

Status: Active » Needs review
FileSize
860 bytes

Are we expecting denominations field with Add more Ajax button ?

In D8 I would go with a commerce_pos_denominations config entity, one per currency, which contains all of the denominations inside

smccabe’s picture

Status: Needs review » Needs work

That would probably make the most sense, although it could also just be a separate add form. The current d7 version just uses a very standard add/edit/delete setup, but it also isn't a config entity.

In short, whichever you feel is easier/better is fine with me.

rakesh.gectcr’s picture

rakesh.gectcr’s picture

Issue summary: View changes
rakesh.gectcr’s picture

Status: Needs work » Needs review
rakesh.gectcr’s picture

Status: Needs review » Needs work

need to add the changes from CurrencyDenominations file as well

rakesh.gectcr’s picture

Status: Needs work » Needs review
FileSize
4.55 KB
596 bytes
swickham’s picture

Status: Needs review » Needs work

Here's what I could find that I think need work:

  • Got an error when trying to save a denomination
    • Drupal\Core\Entity\EntityMalformedException: The entity does not have an ID. in Drupal\Core\Config\Entity\ConfigEntityStorage->save()
  • The $name_field variable is defined in both the main form build and in addmoreCallback() but never used.
  • The "Remove one" button makes it difficult to manage denominations in the form. Instead, there should be a remove button for each denomination, that way if you need to remove one that's higher up in the list you don't need to remove and re-add all the others that come after it.
  • The fields should be required.
  • I noticed there's no currency field like in the screenshot, is that no longer in the plan?
  • Since the addmoreCallback() method is used for both the add and remove actions it should have a more generic name, current one indicates it's only for when something's added to the form.
  • Nothing's calling the validateCurrencyCode() method.
  • Add some descriptions on the fields, it's not clear to an administrator what the fields are for.
rakesh.gectcr’s picture

Status: Needs work » Needs review
FileSize
5.98 KB
3.33 KB

@swickham Thank you, for the review.

I have done all the points which you mentioned above except

The "Remove one" button makes it difficult to manage denominations in the form. Instead, there should be a remove button for each denomination, that way if you need to remove one that's higher up in the list you don't need to remove and re-add all the others that come after it.

We will work on this after the alpha release.
I have created a separated issue as a child of this So that we will not miss it.
#2906918: The "Remove one" button makes it difficult to manage denominations in the form

swickham’s picture

Status: Needs review » Reviewed & tested by the community

Looks good.

smccabe’s picture

Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.