Problem/Motivation

Drupal\commerce_pos_currency_denominations\Form;

Proposed resolution

done

Remaining tasks

User interface changes

API changes

Data model changes

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
StatusFileSize
new860 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

StatusFileSize
new3.82 KB
new59.34 KB
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
StatusFileSize
new4.55 KB
new596 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
StatusFileSize
new5.98 KB
new3.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.