CORE-POS / IS4C

Cooperative Operational Retail Environment
http://www.core-pos.com
GNU General Public License v2.0
63 stars 44 forks source link

BUG: Receipt doesn't show quantities < 0 #661

Closed scannerdarkly closed 8 years ago

scannerdarkly commented 8 years ago

I noticed that if we ring a fractional amount of something (which is how we ring ounces) and it’s less than 1, the display and receipt both omit the price and quantity, showing only the final amount — which is less info than the customer would normally want. I fixed this for George Street Co-op by redefining the rp_ltt_receipt view as shown below (see when abs(itemQtty) NOT IN (0, 1). But because I don’t know all the case branches, I only changed the one line.

  1. Should I check this into the main repository?
  2. Should I modify the other case branches as well?

    use translog;
    DROP VIEW IF EXISTS `rp_ltt_receipt`;
    
    CREATE view rp_ltt_receipt as 
    
    select
    register_no,
    emp_no,
    trans_no,
    description,
    case 
    when voided = 5 
       then 'Discount'
    when trans_status = 'M'
       then 'Mbr special'
    when trans_status = 'S'
       then 'Staff special'
    when scale <> 0 and quantity <> 0 
       then concat(quantity, ' @ ', unitPrice)
    when abs(itemQtty) > 1 and abs(itemQtty) > abs(quantity) and discounttype <> 3 and quantity = 1
       then concat(volume, ' /', unitPrice)
    when abs(itemQtty) > 1 and abs(itemQtty) > abs(quantity) and discounttype <> 3 and quantity <> 1
       then concat(Quantity, ' @ ', Volume, ' /', unitPrice)
    when abs(itemQtty) > 1 and discounttype = 3
       then concat(ItemQtty, ' /', UnitPrice)
    when abs(itemQtty) NOT IN (0, 1)
       then concat(quantity, ' @ ', unitPrice) 
    when matched > 0
       then '1 w/ vol adj'
    else ''
    
    end
    as comment,
    total,
    
    case 
    when trans_status = 'V' 
       then 'VD'
    when trans_status = 'R'
       then 'RF'
    when tax <> 0 and foodstamp <> 0
       then 'TF'
    when tax <> 0 and foodstamp = 0
       then 'T' 
    when tax = 0 and foodstamp <> 0
       then 'F'
    when tax = 0 and foodstamp = 0
       then '' 
    end
    as Status,
    trans_type,
    unitPrice,
    voided,
    trans_id
    
    from localtranstoday
    where voided <> 5 and UPC <> 'TAX' and UPC <> 'DISCOUNT'
    order by emp_no, trans_no, trans_id
scannerdarkly commented 8 years ago

UPDATE: I noticed this change added new lines to the printed receipt (custdata, BADSCAN, etc), probably because I got the view from the old Wedge reference code. Changing the relevant line in InstallUtilities.php, deleting the view, and re-running the install script made for a better change. The original questions apply however!

gohanman commented 8 years ago

RE: question 1: The view in question no longer exists upstream (515e013fe9c391209d666158416f9cb2ddd82ec0). This receipt method has been deprecated for a long time and is officially gone as of 1.10. There's no way to contribute the change to the main repo anymore. You can keep using it you're just on your own maintaining it.

RE: question 2: Well, you're on your own maintaining it :smiley:

scannerdarkly commented 8 years ago

Okay, though it’s still definitely an issue upstream in the screendisplay view and in the modular receipt (which I’ve not yet fixed).

Feel free to cherry-pick two out of three of these: https://github.com/GeorgeStreetCoop/CORE-POS/commit/ac65f6178474ed53cc04681cb5252f8443eab137

gohanman commented 8 years ago

After looking at this further I really don't think it's a bug. It shouldn't be possible to hit that WHEN with a non-integer quantity. By weight items should be handled by the WHEN scale <> 0 AND quantity <> 0 clause. This check specifically prevents such an entry https://github.com/CORE-POS/IS4C/blob/master/pos/is4c-nf/parser-class-lib/parse/UPC.php#L322-L331. I don't think it's a bug when display code doesn't handle a value that isn't allowed.

scannerdarkly commented 8 years ago

Is that code you linked to new? Because at the moment we’re not getting hit with that message when we do an open ring like 0.3*199dp1130 — which is how our co-op enters it when someone buys 0.3 ounces of an herb at $1.99/ounce. (We don’t yet have the herb PLUs in the system.) Or perhaps the code you linked to isn’t exercised on an open ring, and I inadvertently bypassed it.

If the system is going to prevent us from entering fractional ounces entirely, I would consider there to be two bugs at this point!

I haven’t yet started on the high-precision code you and I talked about recently. Will that be the only supported way to enter fractional weights into CORE-POS?

gohanman commented 8 years ago

No; it's been there literally forever. I think the issue is that you're using open rings instead of items. I don't think there's any way to handle open rings with a multiplier that will handle everyone's intended usage cleanly.

On Feb 22, 2016, at 8:04 AM, John Leary notifications@github.com wrote:

Is that code you linked to new? Because at the moment we’re not getting hit with that message when we do an open ring like 0.3*199dp1130 — which is how our co-op enters it when someone buys 0.3 ounces of an herb at $1.99/ounce. (We don’t yet have the herb PLUs in the system.)

If the system is going to prevent us from entering fractional ounces entirely, I would consider there to be two bugs at this point!

I haven’t yet started on the high-precision code you and I talked about recently. Will that be the only supported way to enter fractional weights into CORE-POS?

— Reply to this email directly or view it on GitHub.

scannerdarkly commented 8 years ago

What I could imagine (and this might not fit everyone’s needs but I’ll at least nudge it forward in the attempt to start a dialog) is this: A per-department flag which indicates whether that department will accept non-integer multiples. And that flag applies whether or not it’s an open ring.

The display and receipt printing code would have to be modified as I’d originally been proposing, i.e. all amounts other than 0 and 1 would show “amount @ $price = $total”. That’s pretty quick, I think I’ve literally done that coding in all places but one.

The input side would need to change to look at that per-department flag, and of course for open rings there would need to be a check created where currently there is none.

That said, since George Street Co-op’s usage is out of the current mainstream of this project, I’ll start working on the aforementioned high-precision code change.

gohanman commented 8 years ago

All departments accept non-integer totals with open rings. My point was that it's not guaranteed that the multiplier represents a weight. Something like 0.90*199dp1130 could mean 0.90 pounds or ounces but it could just as easily mean the user is trying to calculate a 10% discount.

There's already a flag for items that indicates whether a non-integer multiple is allowed: products.scale.

scannerdarkly commented 8 years ago

True on the first point, though now that I know about PD I’ll be training cashiers to use that instead for discounted line items. What I don’t understand is this: Why does the system permit non-integers on open rings but not on item rings? Is there some security implication here?

Regarding your second point, it was my understanding that there was no supported way to satisfy all of the following constraints on a non-open-ring PLU item:

Unfortunately, that is precisely the behavior needed for George Street Co-op to track its herb sales by the tenth-ounce; the limitation I’m describing led me to start https://github.com/CORE-POS/IS4C/issues/631 — and to do the code changes I just posted to that thread. I sure hope I didn’t do a bunch of unnecessary coding.

gohanman commented 8 years ago

It's strictly data integrity. A fractional quantity for a non-scale item makes no sense. You can't ring in a half a can of beans or two and a third bags of chips. The system does allow non-integers on items marked as scaleable. There's no restriction whatsoever on values greater than one. 1.5*4011 has worked forever so long as 4011 is correctly entered as a scale item.

On Mon, Feb 22, 2016 at 8:22 PM, John Leary notifications@github.com wrote:

True on the first point, though now that I know about PD I’ll be training cashiers to use that instead for discounted line items. What I don’t understand is this: Why does the system permit non-integers on open rings but not on item rings? Is there some security implication here?

Regarding your second point, it was my understanding that there was no supported way to get cashier-entered non-integer multiples greater than 1 on non-open PLU rings (like George Street Co-op’s herb sales); this led me to start #631 https://github.com/CORE-POS/IS4C/issues/631 — and to do the code changes I just posted to that thread. I sure hope I didn’t do a bunch of unnecessary coding.

— Reply to this email directly or view it on GitHub https://github.com/CORE-POS/IS4C/issues/661#issuecomment-187488404.

scannerdarkly commented 8 years ago

Sorry, I revised my comments for clarity while you replying. Hopefully I’ve made it clear that a fractional quantity for a non-scale item makes much sense, provided the scale can’t be physically connected to the POS terminal. :)

scannerdarkly commented 8 years ago

Another thing which will probably surprise you, and is perhaps unique to George Street Co-op. For years, we have permitted customers to buy one stick of butter out of a four-pack, or one ice cream bar out of a six-pack. A fractional quantity for a non-scale item — and in the latter case, a repeating decimal!

As you’ve described, these can be rung as an open ring. But I don’t understand why that should be necessary. These are, in fact, fractional adjustments to inventory on a UPC-tracked product — just like tenths of ounces, and just like pounds when rung on the scanner scale. I haven’t used the live inventory module yet, but surely it handles fractional pounds accurately when customers buy bulk items, right?

Why should it be different for tenths of ounces? Or for six-packs of ice cream bars?

And if it makes no sense to handle this for an individual UPC (or PLU), why does it make more sense for an open ring?

gohanman commented 8 years ago

You're right that the exact combination you're looking for does not exist, but you're also conflating things that have nothing to do with one another. The entire "count @ $price = $total" tangent has nothing to do with displaying items. The specific reminder prompt that you're triggering is not allowing tenths of ounces, but alternate ways of entering them (i.e., 0.3*1234) is fine. The input restrictions intended to prevent mistakes are the issue, not the underlying display code.

I still disagree very strongly. Even items that will be weighed on an external scale should be marked as scaleable. The alternative is to allow fractional weights on all items which is clearly wrong. Even in your butter/ice cream examples, I think the system should explicitly define which items may have a fractional quantity. Unless it's literally allowed on every item, the system should define which items can and cannot have fractional quantities to prevent entry mistakes.

I don't understand your inventory paragraph at all. Fractional quantities are permitted on items that are marked as scaleable. This includes, of course, bulk items that are sold by weight. Mark ice cream bars as scaleable if you want. Although FDA labeling requirements may be one pretty good reason not to allow fractional quantities on packaged items.

Open rings are an entry method of last resort. At that point the cashier has decided they're going to force a particular price and the multiplier is just a convenience. It's an entirely different scenario than ringing in a known item with well defined attributes.

scannerdarkly commented 8 years ago

I think I was misunderstanding you yesterday. :)

gohanman commented 8 years ago

(Replying to a post that isn't here yet; github's being weird)

For non-integer multiples from a non-connected scale, the system still permits the cashier to enter multiple*PLU — and if we need to force after-the-PLU weight entry, we set both qttyEnforced and scale to 1. Only problem is, this data entry mode has a highly unusual constraint: It only accepts quantities less than one, and with an implied (= system-forced) decimal point!

Why is there such a constraint on just this one data entry mode, when five other modes and sub-modes don’t have such a constraint? Probably because one co-op had one scale and wanted to make it work. Which is understandable as a historical factor, but doesn’t necessarily make sense as a permanent system limitation.

This is more-or-less correct. My only addendum would be it's a highly unusual mode of operation in the first place. For a little over a decade setting both qttyEnforced and scale to 1 wasn't a valid configuration for an item at all. On the first (and prior to this point, only) scenario where a store needed an external scale this mode was added to support it with input restraints tailored to that scenario (very light items that require an additional digit of precision).

As outlined in the other issue, I think it makes more sense to make one lane-side setting that controls the behavior of this input mode than an item-level attribute to do the same. It's probably not a good idea to use multiple input formats for this in the same store and expect cashiers to keep them straight. Maintaining a flag on every item is also more work than changing one lane setting.

At whatever point a third person comes along needing an external scale we can vote on what the default setting should be (provided they're not asking for another new option), but I don't see any reason to break backward compatibility.