Feature: redesign orderscreen - #1213
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📝 WalkthroughWalkthroughAdds folder-based organization for product prices: DB migrations and schema updates, new ProductPriceFolder model and associations, controller endpoints and policies for folder and product-price management (create/update/destroy/reorder/assign), frontend SortableJS-powered UI and JS logic, view and stylesheet changes, and test factory. Changes
Sequence DiagramssequenceDiagram
participant User
participant Client as Browser (order_screen.js)
participant API as Rails API
participant DB as Database
User->>Client: Open "Nieuwe map" modal / submit folder form
Client->>API: POST /price_lists/:id/product_price_folders {name,color}
activate API
API->>API: authorize (treasurer?)
API->>DB: INSERT product_price_folder (set default position)
DB-->>API: folder row
API-->>Client: 201 created with folder JSON
deactivate API
Client->>Client: add folder to folders array, close modal, refresh UI
sequenceDiagram
participant User
participant Client as Browser (order_screen.js)
participant Sortable as SortableJS
participant API as Rails API
participant DB as Database
User->>Client: Drag product into folder tile
Client->>Sortable: reports drop target
Client->>Client: compute target position, call assignProductToFolder
Client->>API: PATCH /product_prices/:id/assign_folder {product_price_folder_id, position}
activate API
API->>API: authorize (treasurer?)
API->>DB: UPDATE product_prices (folder_id, position)
DB-->>API: updated product_price JSON
API-->>Client: updated product_price JSON
deactivate API
Client->>Client: update local product lists and UI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## staging #1213 +/- ##
===========================================
- Coverage 77.59% 69.38% -8.22%
===========================================
Files 54 58 +4
Lines 1406 1597 +191
===========================================
+ Hits 1091 1108 +17
- Misses 315 489 +174 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…alpha/sofia into feature/redesign-orderscreen
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In `@app/controllers/product_price_folders_controller.rb`:
- Around line 33-40: The destroy action currently uses update_all which skips
callbacks and leaves orphaned ProductPrice positions unchanged, risking
duplicate positions at the root; change the logic in
ProductPriceFoldersController#destroy to run inside a transaction, collect the
folder's product_prices (e.g. `@folder.product_prices.order`(:position).to_a), set
each record's product_price_folder_id to nil and recompute/save a new position
for each (e.g. start from ProductPrice.where(product_price_folder_id:
nil).maximum(:position).to_i + 1 and assign incrementally) so callbacks run and
positions are unique, then destroy `@folder`; ensure you use model save!/update!
so validations/callbacks execute.
In `@app/controllers/product_prices_controller.rb`:
- Around line 11-16: Replace the dangerous ProductPriceFolder.find(folder_id)
call with a safe lookup (e.g., ProductPriceFolder.find_by(id: folder_id)) and if
the lookup returns nil, render a validation error JSON (same shape as current
error) with status :unprocessable_entity; otherwise continue to compare the
found folder.price_list_id to `@product_price.price_list_id` and render the
existing "Folder does not belong to the same price list" error when they
mismatch. Ensure you reference folder_id, ProductPriceFolder (use find_by),
folder.price_list_id and `@product_price.price_list_id` in the updated logic.
In `@app/javascript/order_screen.js`:
- Around line 411-419: onDropOnBackButton currently sets the dragged item's
position to this.productPrices.filter(pp => !pp.product_price_folder_id).length
which is off-by-one because the filter includes the dragged item after clearing
product_price_folder_id; update the assignment so productPrice.position =
this.productPrices.filter(pp => !pp.product_price_folder_id).length - 1 (or
compute the count before mutating productPrice) to ensure the dragged item lands
at the final valid index after removing the folder id.
- Around line 315-337: The folder drag handler isn't wired: initialize Sortable
for folders in initSortable by creating this.folderSortableInstance =
Sortable.create(this.$refs.folderContainer, { animation: 100, ghostClass:
'sortable-ghost', chosenClass: 'sortable-chosen', dragClass: 'sortable-drag',
forceFallback: false, touchStartThreshold: 0, delayOnTouchOnly: true, delay: 50,
onEnd: this.onFolderDragEnd.bind(this) }) when this.$refs.folderContainer exists
and folderSortableInstance is null, and in destroySortable call
this.folderSortableInstance.destroy() and set it to null; also ensure template
wraps folder tiles in an element with ref="folderContainer" so the ref exists
for initSortable to use.
- Around line 339-360: The onProductDragEnd handler currently builds
product_positions from the computed visibleProducts (which are sorted by
position) causing incorrect reorder data; instead, read the DOM order from the
Sortable container like onFolderDragEnd. In onProductDragEnd, use
evt.to.querySelectorAll('[data-product-price-id]') (or similar selector) to
iterate DOM nodes in order, map their dataset.productPriceId to build
productPositions = [{ id, position: index }], update the corresponding
productPrices entries' position to the new index, then call the same api.patch
with product_positions and folder_id; keep the existing error handling via
this.handleXHRError.
In `@app/models/product_price.rb`:
- Around line 24-30: The guard in set_default_position currently skips using an
explicitly set position only when position > 0, causing an explicit 0 to be
recalculated; change the guard to preserve any explicitly assigned position
(including 0) by returning early when position is present/not nil (e.g., return
if position.present? or return unless position.nil?), leaving the rest of the
method (scope, max_position calculation using price_list,
product_price_folder_id, maximum(:position), and assigning self.position)
unchanged.
In `@app/views/activities/order_screen.html.erb`:
- Around line 115-124: The SumUp confirm button currently only uses a CSS class
to look disabled which doesn't prevent clicks; update the first button (the one
that calls confirmOrder(true)) to use the actual disabled attribute bound to the
same condition as the dark button (orderConfirmButtonDisabled || editMode) —
i.e. add :disabled="orderConfirmButtonDisabled || editMode" to the button (you
may keep the visual :class binding if desired) so confirmOrder(true) cannot fire
when editMode or submission is disabled.
In `@package.json`:
- Line 37: Update the sortablejs dependency version in package.json from
"sortablejs": "^1.15.0" to "sortablejs": "^1.15.6"; after changing the
"sortablejs" entry, run your package manager (npm install or yarn install) to
refresh node_modules and the lockfile, then run the test/build scripts to verify
no regressions from the upgrade.
🧹 Nitpick comments (3)
db/migrate/20260121000002_add_folder_and_position_to_product_prices.rb (1)
1-7: Consider backfilling position values for existing product_prices.Existing rows will all receive
position: 0after this migration, which may result in undefined ordering when multiple products share the same folder (or no folder). If deterministic ordering matters, consider adding a data migration to assign sequential positions based on existing order (e.g., byidorproduct_id).Example backfill approach
# In a separate data migration or rake task after this schema migration: PriceList.find_each do |pl| pl.product_price.where(product_price_folder_id: nil).order(:id).each_with_index do |pp, idx| pp.update_column(:position, idx) end endapp/models/product_price.rb (1)
14-14: Consider avoidingdefault_scopefor ordering.Using
default_scopefor ordering can lead to unexpected behavior in complex queries, particularly with joins, unions, or when usingunscoped. A safer alternative is to use explicit scopes or to callorder(:position)where needed.♻️ Optional refactor to use explicit scope
- default_scope { order(:position) } + scope :ordered, -> { order(:position) }Then apply
.orderedexplicitly where position ordering is required.app/controllers/product_price_folders_controller.rb (1)
8-8: Redundant.order(:position)call.The
ProductPriceFoldermodel already hasdefault_scope { order(:position) }, making this explicit.order(:position)call redundant.♻️ Proposed fix
- `@folders` = `@price_list.product_price_folders.order`(:position) + `@folders` = `@price_list.product_price_folders`
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@app/javascript/order_screen.js`:
- Around line 414-448: The onDropOnFolder method contains duplicated logic and a
missing closing brace for the forEach callback; remove the duplicated block (the
second productPrice/folderId/productsInFolder/maxPosition section) so there is
only one declaration and one forEach, ensure the forEach callback is closed with
the missing "});" before you continue to set
productPrice.product_price_folder_id and productPrice.position, and keep the
existing api.patch(...) call and its catch intact; verify variables
productPrice, folderId, productsInFolder, and maxPosition are declared only once
within onDropOnFolder.
- Around line 54-57: The data object is missing the folderSortableInstance
reactive property which causes runtime errors when initSortable() and
destroySortable() reference it; add folderSortableInstance: null (or appropriate
initial value) to the component's data return so Vue can track it, then ensure
initSortable() assigns the Sortable instance to this.folderSortableInstance and
destroySortable() checks and clears this.folderSortableInstance when destroying
the instance.
| folderForm: { name: '', color: '#6c757d' }, | ||
| draggedItem: null, | ||
| sortableInstance: null | ||
| }; |
There was a problem hiding this comment.
Missing folderSortableInstance property in data declaration.
folderSortableInstance is referenced in initSortable() (line 331) and destroySortable() (line 351), but it's not declared in the data object. This will cause Vue reactivity issues and a potential runtime error on first access.
🐛 Proposed fix
draggedItem: null,
- sortableInstance: null
+ sortableInstance: null,
+ folderSortableInstance: null
};🤖 Prompt for AI Agents
In `@app/javascript/order_screen.js` around lines 54 - 57, The data object is
missing the folderSortableInstance reactive property which causes runtime errors
when initSortable() and destroySortable() reference it; add
folderSortableInstance: null (or appropriate initial value) to the component's
data return so Vue can track it, then ensure initSortable() assigns the Sortable
instance to this.folderSortableInstance and destroySortable() checks and clears
this.folderSortableInstance when destroying the instance.
| onDropOnFolder(evt, folder) { | ||
| evt.preventDefault(); | ||
| evt.stopPropagation(); | ||
| if (!this.draggedItem || !folder) return; | ||
|
|
||
| const productPrice = this.draggedItem; | ||
| const folderId = parseInt(folder.id); | ||
|
|
||
| const productsInFolder = this.productPrices.filter(pp => pp.product_price_folder_id == folderId); | ||
| let maxPosition = -1; | ||
| productsInFolder.forEach(pp => { | ||
| if (typeof pp.position === 'number' && pp.position > maxPosition) { | ||
| maxPosition = pp.position; | ||
| } | ||
|
|
||
| const productPrice = this.draggedItem; | ||
| const folderId = parseInt(folder.id); | ||
|
|
||
| // Determine the next available position within the target folder | ||
| const productsInFolder = this.productPrices.filter(pp => pp.product_price_folder_id == folderId); | ||
| let maxPosition = -1; | ||
| productsInFolder.forEach(pp => { | ||
| if (typeof pp.position === 'number' && pp.position > maxPosition) { | ||
| maxPosition = pp.position; | ||
| } | ||
| }); | ||
|
|
||
| productPrice.product_price_folder_id = folderId; | ||
| productPrice.position = maxPosition + 1; | ||
| api.patch(`/product_prices/${productPrice.id}/assign_folder`, { | ||
| folder_id: folder.id | ||
| }).catch((response) => { | ||
| this.handleXHRError(response); | ||
| }); | ||
| }, |
There was a problem hiding this comment.
Critical syntax error: duplicate code block and missing closing brace.
The onDropOnFolder method contains duplicated code and a missing }); to close the forEach callback. Lines 419-427 and 429-447 contain the same logic duplicated, and the first forEach block (line 424) is never properly closed. This will cause JavaScript parse errors and prevent the entire file from loading.
Static analysis confirms parse errors at lines 451-452 due to this malformed function.
🐛 Proposed fix — remove duplicate code and fix syntax
onDropOnFolder(evt, folder) {
evt.preventDefault();
evt.stopPropagation();
if (!this.draggedItem || !folder) return;
const productPrice = this.draggedItem;
const folderId = parseInt(folder.id);
- const productsInFolder = this.productPrices.filter(pp => pp.product_price_folder_id == folderId);
- let maxPosition = -1;
- productsInFolder.forEach(pp => {
- if (typeof pp.position === 'number' && pp.position > maxPosition) {
- maxPosition = pp.position;
- }
-
- const productPrice = this.draggedItem;
- const folderId = parseInt(folder.id);
-
// Determine the next available position within the target folder
const productsInFolder = this.productPrices.filter(pp => pp.product_price_folder_id == folderId);
let maxPosition = -1;
productsInFolder.forEach(pp => {
if (typeof pp.position === 'number' && pp.position > maxPosition) {
maxPosition = pp.position;
}
});
productPrice.product_price_folder_id = folderId;
productPrice.position = maxPosition + 1;
api.patch(`/product_prices/${productPrice.id}/assign_folder`, {
folder_id: folder.id
}).catch((response) => {
this.handleXHRError(response);
});
},🧰 Tools
🪛 Biome (2.3.13)
[error] 425-425: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
🤖 Prompt for AI Agents
In `@app/javascript/order_screen.js` around lines 414 - 448, The onDropOnFolder
method contains duplicated logic and a missing closing brace for the forEach
callback; remove the duplicated block (the second
productPrice/folderId/productsInFolder/maxPosition section) so there is only one
declaration and one forEach, ensure the forEach callback is closed with the
missing "});" before you continue to set productPrice.product_price_folder_id
and productPrice.position, and keep the existing api.patch(...) call and its
catch intact; verify variables productPrice, folderId, productsInFolder, and
maxPosition are declared only once within onDropOnFolder.
…alpha/sofia into feature/redesign-orderscreen
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 20 changed files in this pull request and generated 13 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -1,9 +1,31 @@ | |||
| class ProductPrice < ApplicationRecord | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
There are two consecutive blank lines after the class declaration (lines 2-3). According to Ruby style conventions, there should be only one blank line between class declaration and the first method or association.
| has_many :product_price, dependent: :destroy | ||
| has_many :products, through: :product_price, dependent: :restrict_with_exception |
There was a problem hiding this comment.
The PriceList model uses inconsistent association naming. Line 2 uses the singular 'product_price' while this should be the plural 'product_prices' to follow Rails conventions. This is also inconsistent with line 3 which correctly uses 'products', and line 5 which uses 'product_price_folders'. This incorrect naming is causing errors in the ProductPriceFoldersController (line 37) which tries to call '.product_price.without_folder' on the association.
| has_many :product_price, dependent: :destroy | |
| has_many :products, through: :product_price, dependent: :restrict_with_exception | |
| has_many :product_prices, dependent: :destroy | |
| has_many :products, through: :product_prices, dependent: :restrict_with_exception |
|
|
||
| ActiveRecord::Base.transaction do | ||
| folder_positions.each do |folder_data| | ||
| folder = @price_list.product_price_folders.find(folder_data[:id]) |
There was a problem hiding this comment.
The reorder action only authorizes at the class level (ProductPriceFolder) but doesn't authorize each individual folder being reordered. While this might be acceptable since all folders belong to the same price list, consider adding authorization for each folder to ensure consistency with the ProductPricesController pattern (line 32 in product_prices_controller.rb), which authorizes each product_price individually in its reorder action.
| folder = @price_list.product_price_folders.find(folder_data[:id]) | |
| folder = @price_list.product_price_folders.find(folder_data[:id]) | |
| authorize folder, :reorder? |
| @@ -90,13 +90,21 @@ def order_screen # rubocop:disable Metrics/MethodLength, Metrics/AbcSize | |||
| .find(params[:id]) | |||
There was a problem hiding this comment.
This line uses the incorrect association name '.product_price' (singular) which should be '.product_prices' (plural) to match the corrected association in the PriceList model. The includes statement uses the singular 'product_price' which is incorrect and should be 'product_prices'.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@app/controllers/product_price_folders_controller.rb`:
- Around line 33-46: The destroy action references the wrong association name
and lacks transactional safety: change the call from
`@folder.price_list.product_price` to `@folder.price_list.product_prices` (plural)
when computing max_position, and wrap the reassignment loop and `@folder.destroy`
inside a single database transaction (e.g., ApplicationRecord.transaction or
PriceList.transaction) so that updating orphaned_products (product_price
updates) and deleting the folder are atomic and roll back on failure.
In `@app/controllers/product_prices_controller.rb`:
- Around line 26-43: The reorder action updates
product_price.product_price_folder_id from unvalidated params[:folder_id],
allowing assignment to folders outside `@price_list`; fix it by validating each
product_data[:folder_id] belongs to `@price_list` before calling
product_price.update!: for each product_data in reorder, if folder_id is
present, look up the folder via
`@price_list.product_price_folders.find`(product_data[:folder_id]) (or raise
ActiveRecord::RecordNotFound) and use that id (or nil) when setting
product_price_folder_id, keeping the existing authorize(product_price, :update?)
and transaction behavior; ensure you handle missing/null folder_id appropriately
and return the same error handling on failure.
🧹 Nitpick comments (1)
app/javascript/order_screen.js (1)
450-465: Optimistic state mutation without rollback on API failure.
onDropOnBackButton(and similarlyonDropOnFolder) mutatesproductPrice.product_price_folder_idandpositionbefore the API call completes. If theassign_folderPATCH fails, the client state will diverge from the server with no rollback. Consider either saving the previous state and restoring on.catch, or switching to the same pattern used inassignProductToFolder(lines 403–411) which only mutates state in.then().
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This PR will introduce big changes to the orderscreen
It will add the option to group products in folders
it wil also make the location of items moveable
Summary by CodeRabbit
New Features
Style