Comments

TimRutherford created an issue. See original summary.

TimRutherford’s picture

Status: Active » Needs work
StatusFileSize
new15.66 KB

So here's what I got so far.

1) The YAML plugin for Label Formats. Defined formats are in commerce_pos_label.label_formats.yml.

2) The basic form structure for the Print Labels form, currently at admin/commerce/pos/labels or admin/commerce/pos/labels/{product_variation_id}. If given an product variation ID it will load the product and fill in the fields with its information, otherwise you'll get the search product form (not currently functional).

None of the print/add/remove functionality is done.

Nothing with commerce_pos_label_barcode is done.

TimRutherford’s picture

Assigned: TimRutherford » Unassigned
cornifex’s picture

StatusFileSize
new15.73 KB

Patch from #2 with working entity_autocomplete field in print form. Currently only autocompletes variation title. Working on a selection handler that will grab a variation by either title or sku.

cornifex’s picture

StatusFileSize
new6.58 KB

Submit/Validate handlers fleshed out a bit. Still working on the "Remove" handler not firing.

joshmiller’s picture

Assigned: Unassigned » joshmiller

Working on this over the weekend. Goal is to have as much code / feedback / fixes as possible.

joshmiller’s picture

Attached is the combined patches from #4 and #5. Very confused for awhile since the latest patch didn't have new module files :D. Still WIP, nothing new in this patch.

joshmiller’s picture

StatusFileSize
new18.17 KB
smccabe’s picture

gauravjeet’s picture

StatusFileSize
new2.29 KB

Generating barcodes based off product title is done. Currently, it only works with the route commerce_pos_label.print_label.
Added an interdiff of the work done.
For the label printing part, are we thinking of using a print library like mPDF, or is it just a barcode shown on a blank page that can be printed out using local system print?

gauravjeet’s picture

StatusFileSize
new34.68 KB

A screenshot of generated barcode.

barcode

smccabe’s picture

I think we can just format the label like a twig template, thats what we are doing for receipts as well, see #2901030: Receipt Printing

gauravjeet’s picture

StatusFileSize
new19.27 KB

Patch for suggested changes! The patch combines everything from #8.
Again, it only works with route commerce_pos_label.print_label.
Interdiff uploaded in #10

travis-bradbury’s picture

travis-bradbury’s picture

StatusFileSize
new18.3 KB
🐧 phpcs --standard=Drupal --extensions=php,module,inc,install,test,profile,theme,info,txt,md label/

FILE: ...les/contrib/commerce_pos/modules/label/commerce_pos_label.module
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 59 | ERROR | [x] Expected newline after closing brace
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: .../contrib/commerce_pos/modules/label/src/Form/PrintLabelsForm.php
----------------------------------------------------------------------
FOUND 55 ERRORS AND 3 WARNINGS AFFECTING 57 LINES
----------------------------------------------------------------------
   1 | ERROR   | [x] The PHP open tag must be followed by exactly one
     |         |     blank line
   2 | ERROR   | [x] Namespaced classes, interfaces and traits should
     |         |     not begin with a file doc comment
  13 | WARNING | [x] Unused use statement
  16 | ERROR   | [x] Missing class doc comment
  35 | ERROR   | [x] Short array syntax must be used to define arrays
  44 | ERROR   | [x] Short array syntax must be used to define arrays
  48 | ERROR   | [x] Short array syntax must be used to define arrays
  65 | ERROR   | [x] Short array syntax must be used to define arrays
  74 | ERROR   | [x] Short array syntax must be used to define arrays
  81 | ERROR   | [x] Short array syntax must be used to define arrays
  82 | ERROR   | [x] Short array syntax must be used to define arrays
  90 | ERROR   | [x] Inline comments must end in full-stops,
     |         |     exclamation marks, colons, question marks, or
     |         |     closing parentheses
  91 | ERROR   | [x] Short array syntax must be used to define arrays
  94 | ERROR   | [x] Short array syntax must be used to define arrays
  95 | ERROR   | [x] Short array syntax must be used to define arrays
  96 | ERROR   | [x] Short array syntax must be used to define arrays
  97 | ERROR   | [x] Short array syntax must be used to define arrays
  99 | ERROR   | [x] Short array syntax must be used to define arrays
 106 | ERROR   | [x] Short array syntax must be used to define arrays
 112 | ERROR   | [x] Short array syntax must be used to define arrays
 119 | ERROR   | [x] Whitespace found at end of line
 120 | ERROR   | [x] Short array syntax must be used to define arrays
 128 | ERROR   | [x] Short array syntax must be used to define arrays
 135 | ERROR   | [x] Short array syntax must be used to define arrays
 141 | ERROR   | [x] Short array syntax must be used to define arrays
 150 | ERROR   | [x] Inline comments must end in full-stops,
     |         |     exclamation marks, colons, question marks, or
     |         |     closing parentheses
 151 | ERROR   | [x] Short array syntax must be used to define arrays
 155 | ERROR   | [x] Short array syntax must be used to define arrays
 156 | ERROR   | [x] Short array syntax must be used to define arrays
 158 | ERROR   | [x] Short array syntax must be used to define arrays
 162 | ERROR   | [x] Short array syntax must be used to define arrays
 163 | ERROR   | [x] Short array syntax must be used to define arrays
 169 | ERROR   | [x] Inline comments must end in full-stops,
     |         |     exclamation marks, colons, question marks, or
     |         |     closing parentheses
 170 | ERROR   | [x] Short array syntax must be used to define arrays
 174 | ERROR   | [x] Short array syntax must be used to define arrays
 178 | ERROR   | [x] Short array syntax must be used to define arrays
 179 | ERROR   | [x] Short array syntax must be used to define arrays
 190 | ERROR   | [ ] Public method name
     |         |     "PrintLabelsForm::generate_barcode" is not in
     |         |     lowerCamel format
 206 | ERROR   | [ ] Missing parameter comment
 206 | ERROR   | [ ] Missing parameter type
 207 | ERROR   | [ ] Missing parameter comment
 209 | ERROR   | [ ] Description for the @return value is missing
 218 | ERROR   | [ ] Missing parameter comment
 219 | ERROR   | [ ] Missing parameter comment
 225 | ERROR   | [x] Inline comments must end in full-stops,
     |         |     exclamation marks, colons, question marks, or
     |         |     closing parentheses
 237 | ERROR   | [ ] Missing parameter comment
 238 | ERROR   | [ ] Missing parameter comment
 244 | WARNING | [ ] Line exceeds 80 characters; contains 91
     |         |     characters
 260 | ERROR   | [ ] Missing parameter comment
 261 | ERROR   | [ ] Missing parameter comment
 270 | ERROR   | [ ] Missing parameter comment
 271 | ERROR   | [ ] Missing parameter comment
 281 | ERROR   | [x] Inline comments must end in full-stops,
     |         |     exclamation marks, colons, question marks, or
     |         |     closing parentheses
 288 | ERROR   | [x] Inline comments must end in full-stops,
     |         |     exclamation marks, colons, question marks, or
     |         |     closing parentheses
 292 | WARNING | [ ] Line exceeds 80 characters; contains 81
     |         |     characters
 294 | ERROR   | [x] Data types in @param tags need to be fully
     |         |     namespaced
 300 | ERROR   | [ ] Protected method name
     |         |     "PrintLabelsForm::build_info_array" is not in
     |         |     lowerCamel format
 301 | ERROR   | [x] Short array syntax must be used to define arrays
----------------------------------------------------------------------
PHPCBF CAN FIX THE 42 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Time: 156ms; Memory: 8Mb

I get an error when I try to print:

    Warning: array_flip(): Can only flip STRING and INTEGER values! in Drupal\Core\Entity\EntityStorageBase->loadMultiple() (line 227 of core/lib/Drupal/Core/Entity/EntityStorageBase.php).

    Drupal\Core\Entity\EntityStorageBase->loadMultiple(Array) (Line: 212)
    Drupal\Core\Entity\EntityStorageBase->load(NULL) (Line: 512)
    Drupal\Core\Entity\Entity::load(NULL) (Line: 226)
    Drupal\commerce_pos_label\Form\PrintLabelsForm->productAddValidate(Array, Object)
    call_user_func_array(Array, Array) (Line: 83)
    Drupal\Core\Form\FormValidator->executeValidateHandlers(Array, Object) (Line: 274)
    Drupal\Core\Form\FormValidator->doValidateForm(Array, Object, 'print_labels_form') (Line: 119)
    Drupal\Core\Form\FormValidator->validateForm('print_labels_form', Array, Object) (Line: 571)
    Drupal\Core\Form\FormBuilder->processForm('print_labels_form', Array, Object) (Line: 314)
    Drupal\Core\Form\FormBuilder->buildForm('print_labels_form', Object) (Line: 74)
    Drupal\Core\Controller\FormController->getContentResult(Object, Object)
    call_user_func_array(Array, Array) (Line: 123)
    Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 576)
    Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 124)
    Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array) (Line: 97)
    Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}()
    call_user_func_array(Object, Array) (Line: 153)
    Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 68)
    Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 57)
    Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 47)
    Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 99)
    Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 78)
    Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 47)
    Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 50)
    Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 23)
    Stack\StackedHttpKernel->handle(Object, 1, 1) (Line: 657)
    Drupal\Core\DrupalKernel->handle(Object) (Line: 19)

    Label format field is required.
    Invalid product.

Here's the form before getting the error:

Label form before getting the error

There's several TODOs in the module, including that one for the description in the image.

I think that supporting other label formats requires a module to support it. Is that the ideal way to set these up? My initial thought was that this is very similar to how the image style supports image styles, which is as config entities. Should a user be able to create their own label type in a form, or only by supporting it in a module?

travis-bradbury’s picture

StatusFileSize
new15.11 KB

This fixes the phpcs issues, adds a menu link, and makes some changes to the form.

There's a few issues:

  • Ajax removes ajax from the form so you can't use the product search more than once.
  • You can't actually print anything yet.

The second issue will mostly be following along with the receipt submodule but I got sidetracked by the ajax problem so I'm just posting what I did so far.

travis-bradbury’s picture

StatusFileSize
new18.32 KB
new15.11 KB

Whoops, uploaded the interdiff as the patch. Trying again.

travis-bradbury’s picture

StatusFileSize
new18.39 KB
new26.56 KB

New patch with a bit more progress.

  • Fixes the Ajax issue on the form by replacing only the label list, not the whole form.
  • Uses a table to show the labels, not
  • Copies a bunch of printing stuff over from the receipt module.

To do

  • When you change a quantity and then add a product the quantity saves but not if you remove some product. Quantity, title, description, and price edits should work on any submission: add, remove, or print.
  • Generate a barcode as part of the label rendering / print process. Right now it's just a test in the form.
  • Get a product's barcode somehow - a UPC field?
  • Finish the label print command and use it.

I'm not sure how it's working out for label formats so far. The plugins have a CSS option but not their own templates, so that might be a bit limiting.

travis-bradbury’s picture

StatusFileSize
new29.92 KB
new14.04 KB

Another work in progress patch. This one does actual printing!

I had to switch the barcode generator to the PNG version instead of HTML. The HTML one wouldn't show up when printing. The D7 version was using PNG anyway.

The print command works fine but the label needs styling. This patch includes the scss file from the d7 module and it looks like it should be OK for our default label format but it should compiled to css specific to that label format, probably not general for the whole module. The label format config also needs to point to that css file.

Barcodes have to be generated from the product's UPC field. We may want handling for UPC-less printing like the D7 module has. Right now the UPC is hard-coded and added in a theme pre-processor. The processing of products selected / configured by the form to rendered labels is very clunky.

There's also still an issue where some form edits are lost:

  1. Add two products.
  2. Change the quantity for one.
  3. Remove the other.
  4. The quantity goes back to its previous value.

The jQuery print library is added in the label module but should be provided by commerce_pos instead: #2924548: Use commerce_pos jQuery print library in receipt submodule.

travis-bradbury’s picture

StatusFileSize
new36.31 KB

This patch:

  • Fixes the ajax issues in #19 by dropping ajax from the form except for the print callback.
  • Cleans up the barcode generation a bit by using the UPC service to get a product's UPC and a barcode generator service to generate the image.
  • Moves the jQuery.print library to the base commerce_pos module.

Still to do:

  • Tests.
  • Write instructions in the README for using the module and adding new label types.
  • Testing print output with the label printer.
  • The CSS should probably be specific to the label format rather than being named commerce_pos_label.css.

Still awkward is that different templates are not supported for other label formats and label formats define dimensions but those aren't used for anything.

travis-bradbury’s picture

StatusFileSize
new36.31 KB
travis-bradbury’s picture

StatusFileSize
new30.01 KB

Screwed up the interdiff, trying again.

heddn’s picture

  1. +++ b/modules/label/commerce_pos_label.module
    @@ -0,0 +1,118 @@
    +    $barcode = $barcodeGenerator->png($variables['barcode']);
    

    Rather than png, can we name the method something less technology specific and then we can swap out the service implementation for lots of different types of formats? png, svg, html, etc?

  2. +++ b/modules/label/commerce_pos_label.module
    @@ -0,0 +1,118 @@
    +      $variables['barcode'] = '<img src="data:image/png;base64,' . base64_encode($barcode) . '">';
    

    I think I would prefer to see this as a just base64 and let the <img> tag stuff come in via a template. That way folks can more easily swap it out for their special use cases.

  3. +++ b/modules/label/css/commerce_pos_label.css
    --- /dev/null
    +++ b/modules/label/css/commerce_pos_label.css.map
    

    This probably shouldn't get added to the repo.

  4. +++ b/modules/label/src/BarcodeGeneratorInterface.php
    @@ -0,0 +1,27 @@
    +  public function png($upc, $widthFactor, $totalHeight);
    

    Can this be more generic. Say, "generate" instead?

  5. +++ b/modules/label/src/Form/PrintLabelsForm.php
    @@ -0,0 +1,383 @@
    +        'class' => ['commerce-pos-label-btn-add commerce-pos-btn fixed-width btn-success'],
    

    Nit: these should be separate array items. Not one long line.

  6. +++ b/modules/label/src/Form/PrintLabelsForm.php
    @@ -0,0 +1,383 @@
    +          'class' => ['commerce-pos-btn fixed-width btn-danger'],
    ...
    +          'class' => ['commerce-pos-btn fixed-width'],
    

    Same here. Separate array items.

smccabe’s picture

Status: Needs work » Needs review
StatusFileSize
new46.72 KB

tbradbury todo:

Tests.
--Add a test similar to the main POS one that tests the form flow, not able to actually test the print, because afaik we can't test that with phpunit. Could possibly add an additional test for the service, but I dunno how to verify a png, maybe just that it exists? thats pretty limited.
Write instructions in the README for using the module and adding new label types.
--Added simple readme, travis you can probably clean this up in a future issue of you want, or when we add custom twig support
Testing print output with the label printer.
--I do not have a label printer
The CSS should probably be specific to the label format rather than being named commerce_pos_label.css.
--Fixed, also created generic styling to use for the form, as the add button needed some styling

tbradbury: "Still awkward is that different templates are not supported for other label formats and label formats define dimensions but those aren't used for anything."

I'd spin off a child issue for that, since this patch is already huge.

Heddn feedback:
#1 switched to generate
#2 agree, moved into template
#3 I'm gonna go with keeping these in, afaik drupal ci has no way to generate them automatically and they are helpful should the need to debug arise, for us or other users.
#4 same as #1, switched to generate
#5 fixed
#6 fixed

I also moved some menus around as you could only get to labels directly, not through the menu system easily.
admin/commerce/pos moved to admin/commerce/pos/main
admin/commerce/pos is now a listing page that shows the main pos and the labels option

Also also, added scss files to the main gulp file.

I think thats probably it.

  • smccabe committed 2fdf211 on 8.x-2.x authored by tbradbury
    Issue #2894039 by tbradbury, cornifex, gauravjeet, TimRutherford,...
smccabe’s picture

Status: Needs review » Fixed

Ok,

  1. reviewed that barcodes show up in print preview properly (Top Gear Top Tip: twig debugging on totally screws up your print views)
  2. made a small change to the composer.json to allow for future versions of the barcode generation library.
  3. removed some debug and commented out code

Imma commit this mother and anything else can happen in follow-up issues, since this is already a big mamma jamma.

Big thanks to everyone that worked on this, it's really a whole big module in one patch PLUS some core module changes. Extra shoutout to Travis who did lots of the fiddly little bits near the end that actually make everything nice but are a pain in the ass.

Wasn't sure who to give extra special and prestigious "Author" credit to, decided on Travis, but many people did a lot of work here so don't feel slighted.

smccabe’s picture

Added issue for the custom twig files that should still be done at some point. Just noting here so it's not lost if anyone follows this thread.

#2933448: Allow for unique twig templates per label type

Status: Fixed » Closed (fixed)

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