Procházet zdrojové kódy

Add analytical revision of v1.3.2 to docs/revise-1.3.2.md

Read-only review of all source files surfacing gaps and inconsistencies:
broken external-order-number search (key mismatch + wrong HPOS hooks),
status-transition timestamps lost under HPOS via update_post_meta(),
missing CSV import dedup, non-atomic bulk export, UTF-8/Excel issues in
exports, and several documentation drift items vs. the original spec.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Dalibor Votruba před 2 měsíci
rodič
revize
1f0ac1aec7
1 změnil soubory, kde provedl 291 přidání a 0 odebrání
  1. 291 0
      studiou-wc-ord-print-statuses/docs/revise-1.3.2.md

+ 291 - 0
studiou-wc-ord-print-statuses/docs/revise-1.3.2.md

@@ -0,0 +1,291 @@
+# Plugin Revision — v1.3.2 (Analytical Review)
+
+**Scope:** Read-only analytical pass over the entire codebase of `studiou-wc-ord-print-statuses` at version 1.3.2. No code changes were made. This document catalogs gaps, inconsistencies, and risks discovered by reviewing each manager class against the plugin header, README, CLAUDE.md, and the original feature specification in `docs/instructions.txt`.
+
+**Reviewer date:** 2026-05-12
+**Files reviewed:**
+- `studiou-wc-ord-print-statuses.php`
+- `includes/class-db-manager.php`
+- `includes/class-order-status-manager.php`
+- `includes/class-order-fields-manager.php`
+- `includes/class-order-search-manager.php`
+- `includes/class-bulk-actions-manager.php`
+- `includes/class-custom-columns-manager.php`
+- `includes/class-import-manager.php`
+- `includes/utils-log.php`
+- `README.md`, `CLAUDE.md`, `docs/instructions.txt`, `docs/translations.txt`
+
+Findings are grouped by severity. The severity reflects user-visible or correctness impact, not implementation effort to fix.
+
+---
+
+## 1. Critical — features that are advertised but do not work
+
+### 1.1 Search by External Order Number is broken (three independent defects)
+
+The feature is documented in README §5 and CLAUDE.md §"Order_Search_Manager", and matches spec item 3 in `docs/instructions.txt`. The current implementation cannot work for the following reasons:
+
+**Defect A — Meta key mismatch (would fail even under legacy CPT)**
+
+- The field is **saved** as `external_ref_ord_no` (no leading underscore):
+  - `class-order-fields-manager.php:58` — `$order->update_meta_data('external_ref_ord_no', …)`
+  - `class-db-manager.php:184` — `$order->update_meta_data('external_ref_ord_no', $row['externalorder'])`
+- The field is **searched** as `_external_ref_ord_no` (with leading underscore):
+  - `class-order-search-manager.php:18`
+  - `class-order-search-manager.php:53` (`'key' => '_external_ref_ord_no'`)
+
+Even if every other hook below were correct, the `meta_query` LIKE match would always return zero rows.
+
+**Defect B — Wrong hook for HPOS environment**
+
+`class-order-search-manager.php:8` hooks `parse_query` and gates with:
+
+```php
+if ('edit.php' !== $pagenow || 'shop_order' !== $typenow || !$query->is_main_query())
+    return $query;
+```
+
+Under HPOS, the orders screen is served from `admin.php?page=wc-orders` (not `edit.php`) and `$typenow` is not `'shop_order'`. The guard always returns early; `meta_query` is never injected. The plugin declares HPOS compatibility, so this code path is dead in the supported configuration.
+
+**Defect C — Search field is mounted as a column renderer, not a toolbar field**
+
+- `class-order-search-manager.php:6` hooks `add_external_ref_ord_no_to_search` to `manage_woocommerce_page_wc-orders_columns` — that filter expects the orders-list columns array, not a "searchable fields" array. The callback appends `_external_ref_ord_no` as if it were a column key, creating a phantom column.
+- `class-order-search-manager.php:7` hooks `add_external_ref_ord_no_search_field` to `manage_woocommerce_page_wc-orders_custom_column` — that hook fires **per table cell**. The callback renders an `<input type="text">`, which (if the typenow guard ever passed) would draw a text input inside every row's cell rather than beside the "Search Order" button as the spec requires.
+
+**Net effect:** The search box does not render in the expected place, the search hook does not fire under HPOS, and even on a legacy CPT setup the meta key mismatch would zero out results. This feature is non-functional.
+
+### 1.2 `Order_Status_Manager::handle_status_transitions()` writes to the wrong table under HPOS
+
+`class-order-status-manager.php:59-62`:
+
+```php
+update_post_meta($order_id, 'to_print_date', current_time('mysql'));
+…
+update_post_meta($order_id, 'in_print_date', current_time('mysql'));
+```
+
+`update_post_meta()` writes to `wp_postmeta`. Under HPOS, WooCommerce reads order meta from `wp_wc_orders_meta`. The result:
+
+- Whenever a status transitions to `to-print` / `in-print` **outside** of the bulk-action or import paths (e.g., manual change from the order edit screen, programmatic transitions from third-party code), the timestamp is written only to `wp_postmeta` and is invisible to `$order->get_meta()`. The custom column and the "Print Information" panel will show empty values.
+- The bulk and import paths use `$order->update_meta_data() + $order->save()` (correct under both storage modes), so they happen to work — masking the bug for the common user flow.
+
+This is a latent HPOS gap directly contradicting CLAUDE.md §"Important Considerations" point 1.
+
+---
+
+## 2. High — correctness, HPOS gaps, spec divergence
+
+### 2.1 Original spec requires CSV row deduplication; implementation does none
+
+`docs/instructions.txt` lines 28 and 35 both state: *"Distinct values from import file by column 'Order No'"*.
+
+`Studiou_DB_Manager::import_inprint_protocol()` and `import_delivered_protocol()` iterate every row and call `wc_get_order()` + `update_status()` + `save()` per occurrence. If the same `order_no` appears twice in a CSV (a realistic provider-side glitch), the order is mutated twice, the success counter increments twice, and the user gets a misleading "X orders updated" total.
+
+### 2.2 CSV header casing — spec vs. implementation discrepancy
+
+Spec (`docs/instructions.txt` lines 27, 33) specifies headers `"Order No"`, `"ExternalOrder"`, `"ExternalOrderDate"`. The code reads `$row['order_no']`, `$row['externalorder']`, `$row['externalorderdate']` (lowercased, no spaces). README documents the lowercase form, matching the code.
+
+This is a documentation/spec drift rather than a code bug, but it means CSV files prepared from the original instructions will silently fail header lookup (`!isset(...)` branch returns the generic "Invalid row format in CSV.").
+
+### 2.3 Bulk export status change is non-atomic with the export
+
+`class-bulk-actions-manager.php:34-50`:
+
+```php
+$orders_data = Studiou_DB_Manager::get_orders_for_printing($post_ids);
+$this->set_status_to_prepare_to_printing($post_ids, $redirect_to); // ← changes status first
+…
+$this->output_csv($csv_data, 'prepare_to_printing_export.csv');     // ← then output (exits)
+```
+
+If the CSV stream fails mid-output (proxy timeout, client disconnect), orders are already marked `to-print`. The user has no recovery signal and the file may be incomplete. If `output_csv` were ever to throw before `exit`, the bulk-action redirect would still claim success.
+
+Additionally, the `$redirect_to` returned from the inner `set_status_to_prepare_to_printing()` call is discarded — the side effect of adding `bulk_action=marked_prepare_to_printing` to the query is wasted.
+
+### 2.4 Export SQL multiplies rows for products in multiple categories
+
+`Studiou_DB_Manager::get_orders_for_printing()` joins `term_relationships`/`term_taxonomy`/`terms` unconstrained. A product mapped to N `product_cat` terms produces N rows in the CSV for the same order line. If the print provider expects one row per line item, the file will contain duplicate quantities. Whether this is intentional is unclear from the spec; the spec says "custom sql 'SELECT * FROM vsu_orders_car_prod_qty_img'" without defining cardinality.
+
+### 2.5 CSV export has no UTF-8 charset signal and no Excel BOM
+
+`Bulk_Actions_Manager::output_csv()` (lines 84-89) sends:
+
+```
+Content-Type: text/csv
+Content-Disposition: attachment; filename="..."
+```
+
+No `charset=utf-8`, no UTF-8 BOM. Czech product/category names containing diacritics will render as mojibake when opened in Excel on Windows (the most likely consumer for a print-shop export). Field separator is comma; Excel in Czech locale prefers semicolon.
+
+### 2.6 `set_order_processing_status()` can demote print-state orders
+
+`class-order-status-manager.php:65-74` forces any payment-complete order into `processing` unless it is already `completed`. If a payment settles late on an order already moved to `to-print` or `in-print` (e.g., a delayed bank transfer), the order is silently rolled back to `processing` and the `to_print_date` / `in_print_date` timestamps remain, leaving the order in an inconsistent state vs. its meta.
+
+The guard should exclude all print statuses, not just `completed`.
+
+### 2.7 Import handlers reject malformed rows but do not validate file structure
+
+`Studiou_DB_Manager::parse_csv()`:
+- Returns an empty array (not `false`) if the file is missing or empty, but the callers check `if (!$csv_data)` — empty array is falsy in PHP, so the "Error parsing CSV file." path is reached. OK in practice, but reliance on falsy-vs-false is fragile.
+- Reads each row with `fgetcsv($handle, 1000, ',')` — the **1000-byte line cap will truncate** any row longer than 1 KB. Long product names or notes in delivered protocols could be silently chopped.
+- Does not check `$_FILES[...]['error']` (handles UPLOAD_ERR_INI_SIZE, etc.).
+- Does not verify the MIME type server-side; the `accept=".csv"` attribute is client-side only.
+- Does not verify the first line is a header (if a CSV without headers is uploaded, `array_combine` may emit a warning and produce nonsense rows).
+
+### 2.8 Bulk handlers lack a capability check
+
+`Bulk_Actions_Manager::handle_bulk_actions()` trusts WordPress's per-screen capability enforcement. WP does check `edit_shop_orders` to display the bulk action menu, but defense in depth would call `current_user_can('edit_shop_orders')` (or `manage_woocommerce`) inside the handler. Compare with `Import_Manager`, which does call `current_user_can('manage_woocommerce')` — the inconsistency itself is worth noting.
+
+---
+
+## 3. Medium — robustness, maintainability, internal consistency
+
+### 3.1 `UtilsLog::message()` is dead code
+
+`includes/utils-log.php:15-27` — the entire body is commented out. The method is called in at least three places (`class-db-manager.php:128, 153`; `class-bulk-actions-manager.php:42`). The user-facing notice intent (`type: info`, `dismissible: true`) is never delivered. Bulk operations therefore have **no visible feedback** in the admin UI for non-debug users.
+
+### 3.2 `UtilsLog::log()` guard is overly strict
+
+`if ( true === WP_DEBUG )` requires `WP_DEBUG` to be the boolean literal `true`. If the user defines `define('WP_DEBUG', 1)` (common in legacy `wp-config.php` examples), nothing logs. The conventional guard is `if ( defined('WP_DEBUG') && WP_DEBUG )`.
+
+### 3.3 `set_orders_to_prepare_printing` logs per iteration; `set_orders_to_in_printing` does not
+
+`class-db-manager.php:120-131` logs entry, success, and not-found for each order. `class-db-manager.php:143-157` logs neither entry, success, nor not-found. Symmetry would aid debugging.
+
+### 3.4 No transaction or state-machine guard on status transitions
+
+`Studiou_DB_Manager::set_orders_to_*()` calls `update_status()` regardless of current state. An order in `cancelled` or `refunded` would be reactivated into `to-print` — almost certainly unintended. The bulk action UI itself does not filter eligible orders.
+
+### 3.5 Plugin-init order risk for translations
+
+`studiou-wc-ord-print-statuses.php:101-107` registers two `plugins_loaded` callbacks. WordPress runs them in registration order. `Order_Status_Manager::__construct()` calls `_x()` / `_n_noop()` inside the constructor (line 9-15). If `Studiou_WC_Ord_Print_Statuses::initPlugin` registers before `studiou_wc_ord_print_statuses_load_textdomain`, those translation calls fire before the text domain is loaded, and untranslated strings are cached. Order is currently: `initPlugin` registered after `load_textdomain`, so OK *today*, but the dependency is implicit and fragile.
+
+### 3.6 SQL injection defense relies on `intval()`, not `$wpdb->prepare()`
+
+`class-db-manager.php:93`:
+
+```php
+WHERE `orders`.`id` IN (" . implode(',', array_map('intval', $order_ids)) . ")
+```
+
+Using `intval()` on each ID is functionally safe — it coerces to integer and emits zero on garbage input. However, CLAUDE.md §"Important Considerations" point 4 misleadingly claims this *"uses `$wpdb->prepare()` implicitly through `intval()`"* — that is not accurate; `$wpdb->prepare()` is never called. The documentation should match what the code actually does.
+
+### 3.7 Logging volume on every request
+
+Every page-load triggers `UtilsLog::log` calls in `init()`, `load_dependencies()`, `initialize_managers()`, `register_custom_order_statuses()`, `register_bulk_actions()`, etc. With `WP_DEBUG_LOG` enabled, the log fills with init noise. Useful logs (status transitions, SQL errors) get drowned. Consider gating init logs behind a higher verbosity flag.
+
+### 3.8 `parse_csv` line length
+
+`fgetcsv($handle, 1000, ',')` — the second parameter is the max line length in bytes. 1000 bytes is short for a row that contains a long order note or product title plus diacritics. Passing `0` removes the cap. Worth raising explicitly.
+
+### 3.9 Imported `external_ref_ord_date` is stored verbatim
+
+`class-db-manager.php:185` stores `$row['externalorderdate']` without any date parsing or validation. If the provider sends `31/12/2025` while WP expects ISO format elsewhere, downstream filters and the order-detail panel may render an empty value. The "Print Information" panel does not display this field at all — it is only readable via direct meta access.
+
+### 3.10 `external_ref_ord_date` is never exposed in the UI
+
+The field is written by `import_inprint_protocol()` but no manager renders it. Neither `Order_Fields_Manager` nor `Custom_Columns_Manager` references it. If the print-shop's order date matters operationally, it is invisible to the operator.
+
+### 3.11 No uninstall / deactivation cleanup
+
+There is no `uninstall.php`, no `register_deactivation_hook`, no `register_uninstall_hook`. Custom post statuses are registered every `init`, so they vanish when the plugin is deactivated — but order meta keys (`to_print_date`, `external_ref_ord_no`, etc.) remain in the DB indefinitely. For long-term hygiene on test sites, consider documenting that removal requires a manual SQL cleanup.
+
+### 3.12 No version constant
+
+The plugin version lives only in the header comment (`Version: 1.3.2`). To reference it in code (e.g., for asset cache-busting or upgrade routines) one must call `get_file_data()`. Defining `define('STUDIOU_WC_OPS_VERSION', '1.3.2');` once in the bootstrap would be a minor consistency improvement and matches the pattern used in sibling plugins per CLAUDE.md.
+
+### 3.13 HPOS column callback parameter naming
+
+`Custom_Columns_Manager::add_custom_shop_order_column_content($column, $post_id)` — under HPOS the second argument is actually a `WC_Order` object, not a post ID. `wc_get_order()` accepts both, so it works, but the parameter name is misleading and shadows the legacy CPT contract. Renaming to `$order_or_id` or capturing both branches explicitly would aid readers.
+
+---
+
+## 4. Low — documentation, cosmetic, style
+
+### 4.1 Translation reference (`docs/translations.txt`) is incomplete
+
+Strings present in code but absent from the EN→CS reference:
+
+- `"Order automatically set to processing by Studiou WC Order Print Statuses plugin."` (`class-order-status-manager.php:72`)
+- `"Payment received, order is now processing."` (`class-order-status-manager.php:70`)
+- `"Search by External Order Number"` (`class-order-search-manager.php:30`)
+- `"You do not have sufficient permissions to access this page."` (`class-import-manager.php:48, 79`)
+- `"Error parsing CSV file."` (`class-import-manager.php:61, 93`)
+- `"External Order Delivered"` field label (`class-order-fields-manager.php:38`)
+- `"In Print Date"` field label (`class-order-fields-manager.php:25`)
+
+These strings would compile into the `.pot` via `wp i18n make-pot` but were never reviewed by the translator.
+
+### 4.2 README/CLAUDE.md claim full HPOS compatibility
+
+Both documents advertise HPOS compatibility without qualification. Given finding §1.1 (search broken under HPOS) and §1.2 (status-transition stamps go to the wrong table), the claim is overstated until those are fixed.
+
+### 4.3 CLAUDE.md is the only place where the SQL-prepare claim appears
+
+Mentioned in §3.6 — `CLAUDE.md` should be corrected regardless of whether the code is changed, because it currently misrepresents the security posture.
+
+### 4.4 Inconsistent comment language
+
+Some class files open with `// visualized` (in `class-bulk-actions-manager.php`, `class-import-manager.php`, `class-order-fields-manager.php`, `class-custom-columns-manager.php`) — apparently a leftover marker from an earlier review pass. It conveys nothing to a fresh reader and should be removed or replaced with a proper docblock.
+
+### 4.5 `Order_Status_Manager::add_custom_order_statuses_to_list` formatting
+
+`class-order-status-manager.php:48-49`:
+
+```php
+            }        }
+```
+
+Two `}` on the same line, asymmetric indentation. Cosmetic only.
+
+### 4.6 No `Network:` declaration in plugin header
+
+Minor — only relevant for multisite installs.
+
+### 4.7 README architecture table omits `Studiou_DB_Manager` and `UtilsLog` from the manager pattern explanation
+
+Already listed in the table but worth noting that DB Manager is a different category (static service) from the other "manager" classes (hook-registering instances). The README treats them as peers.
+
+---
+
+## 5. Spec compliance matrix (against `docs/instructions.txt`)
+
+| Spec item | Requirement | Status | Notes |
+|-----------|-------------|--------|-------|
+| 1 | Custom statuses `wc-to-print`, `wc-in-print` after `wc-processing` | ✅ Implemented | Inserted correctly via `wc_order_statuses` filter. |
+| 2 | Add fields `to-print-date`, `in-print-date`, `external-ref-ord-no`, `external-ref-ord-delivered` | ⚠️ Partial | Stored with underscore keys (acceptable). `external_ref_ord_date` exists in code but is **not in the spec** and not surfaced in UI. |
+| 3 | Search orders by `external-ref-ord-no` near Search Order button | ❌ Broken | See §1.1 — three independent defects. |
+| 4 | Three bulk actions | ✅ Implemented | Behaviour matches; concerns about non-atomic export (§2.3) and category multiplication (§2.4). |
+| 5 | Three columns appended to orders grid | ✅ Implemented | Inserted after `order_total` rather than strictly "at the end", but visible and functional. |
+| 6 | Import InPrint protocol with deduplication | ⚠️ Partial | Import works; **deduplication not implemented** (§2.1). CSV headers diverge from spec casing (§2.2). |
+| 7 | Import Delivered protocol with deduplication | ⚠️ Partial | Same as item 6. |
+
+---
+
+## 6. Risk-prioritised summary
+
+Issues most likely to cause a user-reported bug, ordered by expected impact:
+
+1. **§1.1 Search by External Order Number does not work** (advertised feature, three defects).
+2. **§1.2 Status-transition timestamps lost under HPOS for non-bulk transitions** (silent data loss).
+3. **§2.5 CSV export mojibake / Excel locale mismatch** (operator-facing, daily friction).
+4. **§2.1 Import dedup missing** (silently double-applies status changes).
+5. **§2.6 Payment-complete demotion of print-state orders** (workflow regression).
+6. **§2.3 Bulk export is non-atomic** (rare, but unrecoverable when it triggers).
+7. **§3.1 `UtilsLog::message()` is dead** (no user feedback after bulk actions).
+8. **§3.4 No status-machine guards** (cancelled/refunded orders can be re-activated by bulk action).
+9. **§2.2 CSV header casing drift vs. original spec** (provider-side surprise).
+10. **§3.6 + §4.3 CLAUDE.md misrepresents SQL safety** (documentation, not code).
+
+---
+
+## 7. Out of scope for this review
+
+- Performance profiling under large order volumes (the export query is unbounded).
+- Browser/JS interaction — there is no front-end code in this plugin.
+- Cross-plugin compatibility with `studiou-wc-product-cat-manage` or `studiou-wc-free-photo-product` (mentioned as siblings in recent git history).
+- Functional testing in a live WP/WC install. All findings above are derived from source reading.
+
+---
+
+*End of review.*