In the Cash count section, there you can add up all the coins/cash in your till, but this is specific to US users and doesn't support other values (like the Canadian $2 coin). This should be improved to be configurable, as lots of places could have different cash values.

Latest changes can be found in this PR: https://github.com/AcroMedia/commerce_pos/pull/42.

CommentFileSizeAuthor
#16 configurable-currency-denominations_2862645_16.patch30.58 KBshabana.navas
#14 configurable-currency-denominations_2862645_14.patch30.61 KBshabana.navas
#13 configurable-currency-denominations_2862645_13.patch30.15 KBshabana.navas
#8 configurable-currency-denominations_2862645_8.patch29.04 KBshabana.navas
#7 Screenshot from 2017-04-21 13-32-11.png1.72 KBsmccabe
#7 Screenshot from 2017-04-21 13-31-59.png2.36 KBsmccabe
#7 Screenshot from 2017-04-21 13-31-43.png2.86 KBsmccabe
#6 configurable-currency-denominations_2862645_6.patch28.95 KBshabana.navas
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

smccabe created an issue. See original summary.

smccabe’s picture

smccabe CreditAttribution: smccabe at Acro Commerce commented
shabana.navas’s picture

shabana.navas CreditAttribution: shabana.navas at Acro Commerce commented
Assigned: Unassigned » shabana.navas

For making this configuration, I'm thinking we add the configuration section to the Reporting page. By default, the currency type would be dollars and we add the denominations that currently exist. Then, we give the user the option to add their own additional denominations to it. We also allow the user to add a different currency type and add the denominations that fall under it.

I don't know how big this change should get, should we make this its own entity or just add a couple of tables like this:

  • currency_denomination_type (currency_id, currency_name)
  • currency_denominations (currency_id, denomination_id, name, title, amount)

Any suggestions? For simplicity's sake and for getting this rolled out as quickly as possible, I'm leaning towards the latter.

shabana.navas’s picture

shabana.navas CreditAttribution: shabana.navas at Acro Commerce commented
Issue summary: View changes
shabana.navas’s picture

shabana.navas CreditAttribution: shabana.navas at Acro Commerce commented
Status: Active » Needs work
shabana.navas’s picture

shabana.navas CreditAttribution: shabana.navas at Acro Commerce commented
Status: Needs work » Needs review
FileSize
configurable-currency-denominations_2862645_6.patch28.95 KB

Added module to provide an interface for configuring denominations.

smccabe’s picture

smccabe CreditAttribution: smccabe at Acro Commerce commented
Issue summary: View changes
Status: Needs review » Needs work
FileSize
Screenshot from 2017-04-21 13-31-43.png2.86 KB
Screenshot from 2017-04-21 13-31-59.png2.36 KB
Screenshot from 2017-04-21 13-32-11.png1.72 KB

Overall I think the architecture is good, some small nitpicks so far.


Needs descriptive text added


Can just say "delete" doesn't need to say denomination, makes it more consistent


Missing breadcrumb text to get back to other settings

shabana.navas’s picture

shabana.navas CreditAttribution: shabana.navas at Acro Commerce commented
FileSize
configurable-currency-denominations_2862645_8.patch29.04 KB

Adding new patch with changes made from comments in #7.

shabana.navas’s picture

shabana.navas CreditAttribution: shabana.navas at Acro Commerce commented
Status: Needs work » Needs review
Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

I've reviewed the code and suggested some changes on the PR:

https://github.com/AcroMedia/commerce_pos/pull/36

Overall, it looks very good. I'm going to try testing it now.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

I tried to enable the module and the commerce_pos_keypad module but got an error because of the duplicate .install file as mentioned in the code review. Removing the commerce_pos_keypad.install file fixed it for me.

The Delete Domination issue that smccabe pointed out is still present. I guess the new patch in #8 needs to be applied to the PR still?

Can you roll the patch from #8 into the PR, or submit an interdiff? Thanks :)

shabana.navas’s picture

shabana.navas CreditAttribution: shabana.navas at Acro Commerce commented

Thanks for the review @wildkatana. I need to update the PR. I'll update asap.

shabana.navas’s picture

shabana.navas CreditAttribution: shabana.navas at Acro Commerce commented
FileSize
configurable-currency-denominations_2862645_13.patch30.15 KB

New patch with updated changes from awesome review from @wildkatana in PR.

shabana.navas’s picture

shabana.navas CreditAttribution: shabana.navas at Acro Commerce commented
FileSize
configurable-currency-denominations_2862645_14.patch30.61 KB

Added fix to prohibit a denomination type from being deleted if it is part of the configured denom types. PR also updated.

shabana.navas’s picture

shabana.navas CreditAttribution: shabana.navas at Acro Commerce commented
Issue summary: View changes
shabana.navas’s picture

shabana.navas CreditAttribution: shabana.navas at Acro Commerce commented
FileSize
configurable-currency-denominations_2862645_16.patch30.58 KB

Update patch from recommendations in PR.

smccabe’s picture

smccabe CreditAttribution: smccabe at Acro Commerce commented
Issue tags: +rc1 blocker

  • smccabe committed da4a169 on 7.x-2.x authored by shabana.navas 
    Issue #2862645 by shabana.navas, smccabe, wildkatana: Make currency...
smccabe’s picture

smccabe CreditAttribution: smccabe at Acro Commerce commented
Status: Needs review » Fixed

Cleaned up some phpcs issues and merged in!

Great job @shabana.navas and @wildkatana! Big patch but it looks very good.

Status: Fixed » Closed (fixed)

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