Clean up sizes/sizegroup: deduplicate sizes + cross-reference table#821
Clean up sizes/sizegroup: deduplicate sizes + cross-reference table#821Copilot wants to merge 19 commits into
Conversation
Agent-Logs-Url: https://github.com/boxwise/dropapp/sessions/870cef39-acd9-49fe-8c16-4dfd54f3b425 Co-authored-by: pylipp <10617122+pylipp@users.noreply.github.com>
Migration 1 (20260421120000): removes 75 duplicate size rows, updating references in stock, shipment_detail, and history tables. Migration 2 (20260421130000): creates sizes_sizegroup cross-reference table with all size-sizegroup relationships (including extra sizegroup memberships from merged duplicates), then drops sizes.sizegroup_id and sizes.seq. Updates include/container-stock.php to JOIN sizes_sizegroup instead of filtering on sizes.sizegroup_id. Agent-Logs-Url: https://github.com/boxwise/dropapp/sessions/870cef39-acd9-49fe-8c16-4dfd54f3b425 Co-authored-by: pylipp <10617122+pylipp@users.noreply.github.com>
…ntainer-stock.php Agent-Logs-Url: https://github.com/boxwise/dropapp/sessions/363f3d93-798a-48aa-b118-9a9a41ecb889 Co-authored-by: pylipp <10617122+pylipp@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Refactors the app’s size/sizegroup data model by removing duplicate sizes rows and moving from a 1→many (sizes.sizegroup_id) relationship to a many→many relationship via a new sizes_sizegroup cross-reference table, updating stockroom querying accordingly.
Changes:
- Add a migration to deduplicate
sizeslabels by remapping foreign keys and deleting duplicate rows. - Add a migration to create/populate
sizes_sizegroup, then dropsizes.sizegroup_idandsizes.seq. - Update the stockroom listing query to join sizes via
sizes_sizegroupinstead ofsizes.sizegroup_id.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| include/container-stock.php | Switches sizegroup matching logic to use the new sizes_sizegroup join. |
| db/migrations/20260421120000_deduplicate_sizes.php | Remaps references to removed size IDs and deletes duplicate sizes rows. |
| db/migrations/20260421130000_create_sizes_sizegroup_table.php | Introduces sizes_sizegroup, migrates memberships, and drops legacy sizes columns. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Philipp Metzner <10617122+pylipp@users.noreply.github.com>
|
@copilot apply changes based on the comments in this thread |
Agent-Logs-Url: https://github.com/boxwise/dropapp/sessions/ce308e1f-588a-4185-aa41-13ca1c14601d Co-authored-by: pylipp <10617122+pylipp@users.noreply.github.com>
…getsizes.php, stock_edit.php, sizes_edit.php to use sizes_sizegroup Agent-Logs-Url: https://github.com/boxwise/dropapp/sessions/ce308e1f-588a-4185-aa41-13ca1c14601d Co-authored-by: pylipp <10617122+pylipp@users.noreply.github.com>
Done in 18aab4f. Applied all three review changes:
|
…ON_SCHEMA Agent-Logs-Url: https://github.com/boxwise/dropapp/sessions/2c1f475d-e3cc-4a30-83d0-3a45e2ba207c Co-authored-by: pylipp <10617122+pylipp@users.noreply.github.com>
…ng them Agent-Logs-Url: https://github.com/boxwise/dropapp/sessions/f5db50ea-15bf-4438-9c1e-b0d0bf82d2d9 Co-authored-by: pylipp <10617122+pylipp@users.noreply.github.com>
Co-authored-by: pylipp <10617122+pylipp@users.noreply.github.com>
|
@HaGuesto the way I tested this was:
I could verify
|
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Philipp Metzner <10617122+pylipp@users.noreply.github.com>
There was a problem hiding this comment.
functional review
Run test queries
- I queried the 10 last rows from history, stock and shipment_detail and exported them.
- I exported the sizes and sizegroup table
##Run first migration
- went through
- as expected the migration did not change the data in my exports
- I also exported the sizegroup_sizes table.
- I found some seq errors and duplication of "All_ages" and "Mixed". Will check again when 2nd migration is applied
CHANGE REQUEST: remove created, created_by, modified, modified_by columns from sizes table.
will continue review in next comment
|
@copilot could you add an migration at the end removing the following columns from the sizes table since they are only filled with NULL: created, created_by, modified, modified_by. |
…ied, modified_by) from sizes table Agent-Logs-Url: https://github.com/boxwise/dropapp/sessions/5a267922-fb0f-4a79-9c7d-04cc5362d322 Co-authored-by: HaGuesto <29065125+HaGuesto@users.noreply.github.com>
Done in 37af64e — added migration |
HaGuesto
left a comment
There was a problem hiding this comment.
functional review continued
Run rollback
- size and sizegroup tables are the same as before
Run first and second migration
- number of sizes reduced from 179 to 113 as expected.
- sizes_sizegroup table lost one enty? There seemed to be a duplicate entry of All Ages in one sizegroup.
- history snapshot of 10 changes was correct
- stock snapshot of 10 changes was correct
- shipment_detail snapshot of 10 changes was correct
@pylipp I found a shipment detail with source_size_id = NULL. does that make sense?
Run rollback of second migration
works
| SET from_int = {$fromCaseExpr} | ||
| WHERE tablename = 'stock' AND changes = 'size_id' | ||
| AND from_int IN ({$ids})"); | ||
|
|
There was a problem hiding this comment.
@pylipp add here a message how many row items should have changed vs have actually changed based on production.
|
@copilot I need another migration to clean-up inconsistencies in the sizes data. It should run fourth.
|
Agent-Logs-Url: https://github.com/boxwise/dropapp/sessions/388acca9-0bf3-4f83-ba03-25ffdb08105d Co-authored-by: pylipp <10617122+pylipp@users.noreply.github.com>
…s 17/18/22/25 Agent-Logs-Url: https://github.com/boxwise/dropapp/sessions/388acca9-0bf3-4f83-ba03-25ffdb08105d Co-authored-by: pylipp <10617122+pylipp@users.noreply.github.com>
Done in 04d0dfc — added migration
|
Implements the size/sizegroup database cleanup.
Migration 1 –
20260421120000_CreateSizesSizegroupTablesizes_sizegroupcross-reference table (many-to-many betweensizesandsizegroup).sizes.sizegroup_idfor every size currently in the table — including the 75 duplicates that Migration 2 will later remove (so no "extra memberships" block is needed).sizegroup_idandseqcolumns fromsizes.The
down()method restoressizegroup_id/seq, re-populates them fromsizes_sizegroup, and drops the cross-reference table.Migration 2 –
20260421130000_DeduplicateSizesRemoves 75 duplicate size rows from the
sizestable (keeping the entry with the smallest ID for each label). Before deleting the duplicates, all foreign-key references are redirected to the surviving ID:stocksize_idshipment_detailsource_size_id,target_size_idhistoryfrom_int,to_int(fortablename='stock',changes='size_id')itemsoutsize_iddistro_events_packing_list_entriessize_iddistro_events_tracking_logssize_iddistro_events_unboxed_item_collectionssize_idsizes_sizegrouprows for duplicate sizes are then remapped to their canonical ID viaINSERT IGNORE(so canonical sizes inherit all sizegroup memberships from their duplicates). TheON DELETE CASCADEonfk_ss_size_idremoves the old duplicate rows when the duplicatesizesrows are deleted.The
down()method re-inserts the 75 deletedsizesrows (id + label only, sincesizegroup_id/seqare managed by Migration 1) and their correspondingsizes_sizegroupentries, then removes the inherited sizegroup memberships from the canonical sizes (partial rollback; FK remappings cannot be reconstructed).Migration 3 –
20260421140000_DropAuditColumnsFromSizesRemoves the unused audit columns (
created,created_by,modified,modified_by) from thesizestable. These columns are allNULLin every known environment and are not referenced by any application code. FK constraints and indexes oncreated_by/modified_byare dropped first.The
down()method restores the columns, indexes, and FK constraints referencingcms_users.Migration 4 –
20260421150000_FixSizesDataCleans up data inconsistencies in the
sizes_sizegrouptable:seqvalues to XS=1, S=2, M=3, L=4, XL=5, XXL=6, Mixed=999.seqvalues to 2-3y=200, 4-5y=201, 6-7y=202, 8-9y=203, 10-11y=204, 12-13y=205, 14-15y=206, All ages=207, Mixed=999.The
down()method removes the added "All ages" entries and restores the originalseqvalues.Code changes
include/container-stock.phpINNER JOIN sizes_sizegroup … INNER JOIN sizeslibrary/ajax/getsizes.phpsizes_sizegroupinclude/stock_edit.phpsizes_sizegroupVerification
vendor/bin/parallel-lint– no errorsphp-cs-fixer– 0 files to fix (excluding auto-generated templates)sizestable contains onlyidandlabelseqvalues; sizegroups 17, 18, 22, and 25 include "All ages"