Feature: redesign orderscreen #1213
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,78 @@ | ||||||||
| class ProductPriceFoldersController < ApplicationController | ||||||||
| before_action :authenticate_user! | ||||||||
| before_action :set_price_list, only: %i[index create reorder] | ||||||||
| before_action :set_folder, only: %i[update destroy] | ||||||||
|
|
||||||||
| def index | ||||||||
| authorize ProductPriceFolder, :index? | ||||||||
| @folders = @price_list.product_price_folders.order(:position) | ||||||||
| render json: @folders | ||||||||
| end | ||||||||
|
|
||||||||
| def create | ||||||||
| @folder = @price_list.product_price_folders.new(folder_params) | ||||||||
| authorize @folder | ||||||||
|
|
||||||||
| if @folder.save | ||||||||
| render json: @folder, status: :created | ||||||||
| else | ||||||||
| render json: { errors: @folder.errors.full_messages }, status: :unprocessable_content | ||||||||
| end | ||||||||
| end | ||||||||
|
|
||||||||
| def update | ||||||||
| authorize @folder | ||||||||
|
|
||||||||
| if @folder.update(folder_params) | ||||||||
| render json: @folder | ||||||||
| else | ||||||||
| render json: { errors: @folder.errors.full_messages }, status: :unprocessable_content | ||||||||
| end | ||||||||
| end | ||||||||
|
|
||||||||
| def destroy | ||||||||
| authorize @folder | ||||||||
|
|
||||||||
| orphaned_products = @folder.product_prices | ||||||||
| max_position = @folder.price_list.product_price.without_folder.maximum(:position) || -1 | ||||||||
|
There was a problem hiding this comment. This line attempts to call '.product_price.without_folder' on the price_list, but the association name 'product_price' is singular and incorrect. The PriceList model (line 2) uses 'has_many :product_price' which should be 'has_many :product_prices' (plural). Once that is fixed, this line should be changed to '@folder.price_list.product_prices.without_folder.maximum(:position)' to match the corrected association name.
Suggested change
|
||||||||
|
|
||||||||
| orphaned_products.each_with_index do |product_price, index| | ||||||||
| product_price.update(product_price_folder_id: nil, position: max_position + index + 1) | ||||||||
| end | ||||||||
|
|
||||||||
| @folder.destroy | ||||||||
|
|
||||||||
| head :no_content | ||||||||
| end | ||||||||
|
Comment on lines
+33
to
+46
There was a problem hiding this comment. Critical bug:
Additionally, the individual product updates (lines 39–41) are not wrapped in a transaction, so a failure mid-way would leave partially reassigned products. 🐛 Proposed fix def destroy
authorize `@folder`
orphaned_products = `@folder.product_prices`
- max_position = `@folder.price_list.product_price.without_folder.maximum`(:position) || -1
+ max_position = `@folder.price_list.product_prices.without_folder.maximum`(:position) || -1
- orphaned_products.each_with_index do |product_price, index|
- product_price.update(product_price_folder_id: nil, position: max_position + index + 1)
- end
-
- `@folder.destroy`
+ ActiveRecord::Base.transaction do
+ orphaned_products.each_with_index do |product_price, index|
+ product_price.update!(product_price_folder_id: nil, position: max_position + index + 1)
+ end
+ `@folder.destroy`!
+ end
head :no_content
end🤖 Prompt for AI Agents |
||||||||
|
|
||||||||
| def reorder # rubocop:disable Metrics/MethodLength | ||||||||
| authorize ProductPriceFolder, :reorder? | ||||||||
|
|
||||||||
| folder_positions = params.require(:folder_positions) | ||||||||
|
|
||||||||
| 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.
Suggested change
|
||||||||
| folder.update!(position: folder_data[:position]) | ||||||||
| end | ||||||||
| end | ||||||||
|
|
||||||||
| render json: { success: true } | ||||||||
| rescue ActiveRecord::RecordInvalid => e | ||||||||
| render json: { errors: [e.message] }, status: :unprocessable_content | ||||||||
| end | ||||||||
|
|
||||||||
| private | ||||||||
|
|
||||||||
| def set_price_list | ||||||||
| @price_list = PriceList.find(params[:price_list_id]) | ||||||||
| end | ||||||||
|
|
||||||||
| def set_folder | ||||||||
| @folder = ProductPriceFolder.find(params[:id]) | ||||||||
| end | ||||||||
|
|
||||||||
| def folder_params | ||||||||
| params.require(:product_price_folder).permit(:name, :color, :position) | ||||||||
| end | ||||||||
| end | ||||||||
lodewiges marked this conversation as resolved.
Show resolved
Hide resolved
Comment on lines
+1
to
+78
There was a problem hiding this comment. The new ProductPriceFoldersController lacks test coverage. The codebase has comprehensive test coverage for other controllers (as evidenced by the spec/controllers directory), but there are no tests for this new controller. Tests should be added to cover the index, create, update, destroy, and reorder actions, including authorization checks for different user roles. |
||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. This will cause a NoMethodError at runtime.