diff --git a/app/controllers/api/v1/valuations_controller.rb b/app/controllers/api/v1/valuations_controller.rb index 495259bf5..ad50e5d92 100644 --- a/app/controllers/api/v1/valuations_controller.rb +++ b/app/controllers/api/v1/valuations_controller.rb @@ -1,10 +1,47 @@ # frozen_string_literal: true class Api::V1::ValuationsController < Api::V1::BaseController - before_action :ensure_read_scope, only: [ :show ] + include Pagy::Backend + + InvalidFilterError = Class.new(StandardError) + + before_action :ensure_read_scope, only: [ :index, :show ] before_action :ensure_write_scope, only: [ :create, :update ] before_action :set_valuation, only: [ :show, :update ] + def index + family = current_resource_owner.family + accessible_account_ids = family.accounts.accessible_by(current_resource_owner).select(:id) + valuations_query = family.entries + .where(entryable_type: "Valuation", account_id: accessible_account_ids) + .includes(:account, :entryable) + + valuations_query = apply_filters(valuations_query).reverse_chronological + @per_page = safe_per_page_param + + @pagy, @entries = pagy( + valuations_query, + page: safe_page_param, + limit: @per_page + ) + + render :index + rescue InvalidFilterError => e + render json: { + error: "validation_failed", + message: e.message, + errors: [ e.message ] + }, status: :unprocessable_entity + rescue => e + Rails.logger.error "ValuationsController#index error: #{e.message}" + Rails.logger.error e.backtrace.join("\n") + + render json: { + error: "internal_server_error", + message: "An unexpected error occurred" + }, status: :internal_server_error + end + def show render :show rescue => e @@ -208,6 +245,43 @@ class Api::V1::ValuationsController < Api::V1::BaseController authorize_scope!(:write) end + def apply_filters(query) + if params[:account_id].present? + raise InvalidFilterError, "account_id must be a valid UUID" unless valid_uuid?(params[:account_id]) + + query = query.where(account_id: params[:account_id]) + end + query = query.where("entries.date >= ?", parse_date_param(:start_date)) if params[:start_date].present? + query = query.where("entries.date <= ?", parse_date_param(:end_date)) if params[:end_date].present? + query + end + + def parse_date_param(key) + Date.iso8601(params[key].to_s) + rescue ArgumentError + raise InvalidFilterError, "#{key} must be an ISO 8601 date" + end + + def valid_uuid?(value) + value.to_s.match?(/\A[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}\z/i) + end + + def safe_page_param + page = params[:page].to_i + page > 0 ? page : 1 + end + + def safe_per_page_param + per_page = params[:per_page].to_i + + case per_page + when 1..100 + per_page + else + 25 + end + end + def valuation_account_id params.dig(:valuation, :account_id) end diff --git a/app/views/api/v1/valuations/_valuation.json.jbuilder b/app/views/api/v1/valuations/_valuation.json.jbuilder index 1e63fb537..3341fa0e6 100644 --- a/app/views/api/v1/valuations/_valuation.json.jbuilder +++ b/app/views/api/v1/valuations/_valuation.json.jbuilder @@ -1,17 +1,19 @@ # frozen_string_literal: true -json.id valuation.entry.id -json.date valuation.entry.date -json.amount valuation.entry.amount_money.format -json.currency valuation.entry.currency -json.notes valuation.entry.notes +entry = local_assigns[:entry] || valuation.entry + +json.id entry.id +json.date entry.date +json.amount entry.amount_money.format +json.currency entry.currency +json.notes entry.notes json.kind valuation.kind # Account information json.account do - json.id valuation.entry.account.id - json.name valuation.entry.account.name - json.account_type valuation.entry.account.accountable_type.underscore + json.id entry.account.id + json.name entry.account.name + json.account_type entry.account.accountable_type.underscore end # Additional metadata diff --git a/app/views/api/v1/valuations/index.json.jbuilder b/app/views/api/v1/valuations/index.json.jbuilder new file mode 100644 index 000000000..a7b93c0d2 --- /dev/null +++ b/app/views/api/v1/valuations/index.json.jbuilder @@ -0,0 +1,12 @@ +# frozen_string_literal: true + +json.valuations @entries do |entry| + json.partial! "valuation", valuation: entry.entryable, entry: entry +end + +json.pagination do + json.page @pagy.page + json.per_page @per_page + json.total_count @pagy.count + json.total_pages @pagy.pages +end diff --git a/config/routes.rb b/config/routes.rb index 544d1c8f0..e6a705346 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -427,7 +427,7 @@ Rails.application.routes.draw do resources :transactions, only: [ :index, :show, :create, :update, :destroy ] resources :trades, only: [ :index, :show, :create, :update, :destroy ] resources :holdings, only: [ :index, :show ] - resources :valuations, only: [ :create, :update, :show ] + resources :valuations, only: [ :index, :create, :update, :show ] resources :imports, only: [ :index, :show, :create ] resource :usage, only: [ :show ], controller: :usage resource :balance_sheet, only: [ :show ], controller: :balance_sheet diff --git a/docs/api/openapi.yaml b/docs/api/openapi.yaml index 5b9000e51..acf51f91c 100644 --- a/docs/api/openapi.yaml +++ b/docs/api/openapi.yaml @@ -579,6 +579,18 @@ components: updated_at: type: string format: date-time + ValuationCollection: + type: object + required: + - valuations + - pagination + properties: + valuations: + type: array + items: + "$ref": "#/components/schemas/Valuation" + pagination: + "$ref": "#/components/schemas/Pagination" DeleteResponse: type: object required: @@ -2952,19 +2964,72 @@ paths: schema: "$ref": "#/components/schemas/ErrorResponse" "/api/v1/valuations": + get: + summary: List valuations + tags: + - Valuations + security: + - apiKeyAuth: [] + parameters: + - name: page + in: query + required: false + description: 'Page number (default: 1)' + schema: + type: integer + - name: per_page + in: query + required: false + description: 'Items per page (default: 25, max: 100)' + schema: + type: integer + - name: account_id + in: query + required: false + description: Filter by account ID + schema: + type: string + format: uuid + - name: start_date + in: query + required: false + description: Filter valuations from this date + schema: + type: string + format: date + - name: end_date + in: query + required: false + description: Filter valuations until this date + schema: + type: string + format: date + responses: + '200': + description: valuations listed + content: + application/json: + schema: + "$ref": "#/components/schemas/ValuationCollection" + '401': + description: unauthorized + content: + application/json: + schema: + "$ref": "#/components/schemas/ErrorResponse" + '422': + description: invalid account filter + content: + application/json: + schema: + "$ref": "#/components/schemas/ErrorResponse" post: summary: Create valuation tags: - Valuations security: - apiKeyAuth: [] - parameters: - - name: Authorization - in: header - required: true - schema: - type: string - description: Bearer token with write scope + parameters: [] responses: '201': description: valuation created @@ -3016,12 +3081,6 @@ paths: required: true "/api/v1/valuations/{id}": parameters: - - name: Authorization - in: header - required: true - schema: - type: string - description: Bearer token - name: id in: path required: true diff --git a/spec/requests/api/v1/valuations_spec.rb b/spec/requests/api/v1/valuations_spec.rb index 04319c688..ca6466042 100644 --- a/spec/requests/api/v1/valuations_spec.rb +++ b/spec/requests/api/v1/valuations_spec.rb @@ -20,25 +20,18 @@ RSpec.describe 'API V1 Valuations', type: :request do ) end - let(:oauth_application) do - Doorkeeper::Application.create!( - name: 'API Docs', - redirect_uri: 'https://example.com/callback', - scopes: 'read read_write' + let(:api_key) do + key = ApiKey.generate_secure_key + ApiKey.create!( + user: user, + name: 'API Docs Key', + key: key, + scopes: %w[read_write], + source: 'web' ) end - let(:access_token) do - Doorkeeper::AccessToken.create!( - application: oauth_application, - resource_owner_id: user.id, - scopes: 'read_write', - expires_in: 2.hours, - token: SecureRandom.hex(32) - ) - end - - let(:Authorization) { "Bearer #{access_token.token}" } + let(:'X-Api-Key') { api_key.plain_key } let(:account) do Account.create!( @@ -66,13 +59,59 @@ RSpec.describe 'API V1 Valuations', type: :request do let!(:valuation_id) { valuation_entry.id } path '/api/v1/valuations' do + get 'List valuations' do + tags 'Valuations' + security [ { apiKeyAuth: [] } ] + produces 'application/json' + parameter name: :page, in: :query, type: :integer, required: false, + description: 'Page number (default: 1)' + parameter name: :per_page, in: :query, type: :integer, required: false, + description: 'Items per page (default: 25, max: 100)' + parameter name: :account_id, in: :query, required: false, description: 'Filter by account ID', + schema: { type: :string, format: :uuid } + parameter name: :start_date, in: :query, required: false, + description: 'Filter valuations from this date', + schema: { type: :string, format: :date } + parameter name: :end_date, in: :query, required: false, + description: 'Filter valuations until this date', + schema: { type: :string, format: :date } + + response '200', 'valuations listed' do + schema '$ref' => '#/components/schemas/ValuationCollection' + + run_test! + end + + response '401', 'unauthorized' do + schema '$ref' => '#/components/schemas/ErrorResponse' + + let(:'X-Api-Key') { nil } + + run_test! + end + + response '422', 'invalid date filter' do + schema '$ref' => '#/components/schemas/ErrorResponse' + + let(:start_date) { 'not-a-date' } + + run_test! + end + + response '422', 'invalid account filter' do + schema '$ref' => '#/components/schemas/ErrorResponse' + + let(:account_id) { 'not-a-uuid' } + + run_test! + end + end + post 'Create valuation' do tags 'Valuations' security [ { apiKeyAuth: [] } ] consumes 'application/json' produces 'application/json' - parameter name: :Authorization, in: :header, required: true, schema: { type: :string }, - description: 'Bearer token with write scope' parameter name: :body, in: :body, required: true, schema: { type: :object, properties: { @@ -170,8 +209,6 @@ RSpec.describe 'API V1 Valuations', type: :request do end path '/api/v1/valuations/{id}' do - parameter name: :Authorization, in: :header, required: true, schema: { type: :string }, - description: 'Bearer token' parameter name: :id, in: :path, type: :string, required: true, description: 'Valuation ID (entry ID)' get 'Retrieve a valuation' do diff --git a/spec/swagger_helper.rb b/spec/swagger_helper.rb index d788ba8e5..123603dbe 100644 --- a/spec/swagger_helper.rb +++ b/spec/swagger_helper.rb @@ -364,6 +364,17 @@ RSpec.configure do |config| updated_at: { type: :string, format: :'date-time' } } }, + ValuationCollection: { + type: :object, + required: %w[valuations pagination], + properties: { + valuations: { + type: :array, + items: { '$ref' => '#/components/schemas/Valuation' } + }, + pagination: { '$ref' => '#/components/schemas/Pagination' } + } + }, DeleteResponse: { type: :object, required: %w[message], diff --git a/test/controllers/api/v1/valuations_controller_test.rb b/test/controllers/api/v1/valuations_controller_test.rb index 0864470c1..125863af4 100644 --- a/test/controllers/api/v1/valuations_controller_test.rb +++ b/test/controllers/api/v1/valuations_controller_test.rb @@ -17,6 +17,7 @@ class Api::V1::ValuationsControllerTest < ActionDispatch::IntegrationTest user: @user, name: "Test Read-Write Key", scopes: [ "read_write" ], + source: "web", display_key: "test_rw_#{SecureRandom.hex(8)}" ) @@ -33,6 +34,92 @@ class Api::V1::ValuationsControllerTest < ActionDispatch::IntegrationTest Redis.new.del("api_rate_limit:#{@read_only_api_key.id}") end + # INDEX action tests + test "should get index with valid API key" do + get api_v1_valuations_url, headers: api_headers(@api_key) + assert_response :success + + response_data = JSON.parse(response.body) + assert response_data.key?("valuations") + assert response_data.key?("pagination") + assert response_data["valuations"].is_a?(Array) + assert response_data["pagination"].key?("page") + assert response_data["pagination"].key?("per_page") + assert response_data["pagination"].key?("total_count") + assert response_data["pagination"].key?("total_pages") + end + + test "should get index with read-only API key" do + get api_v1_valuations_url, headers: api_headers(@read_only_api_key) + assert_response :success + end + + test "should filter index by account_id" do + get api_v1_valuations_url, + params: { account_id: @account.id }, + headers: api_headers(@api_key) + assert_response :success + + response_data = JSON.parse(response.body) + response_data["valuations"].each do |valuation| + assert_equal @account.id, valuation["account"]["id"] + end + end + + test "should filter index by date range" do + entry = @valuation.entry + + get api_v1_valuations_url, + params: { start_date: entry.date, end_date: entry.date }, + headers: api_headers(@api_key) + assert_response :success + + response_data = JSON.parse(response.body) + assert_includes response_data["valuations"].map { |valuation| valuation["id"] }, entry.id + response_data["valuations"].each do |valuation| + valuation_date = Date.iso8601(valuation["date"]) + assert_equal entry.date, valuation_date + end + end + + test "should reject index with invalid date filter" do + get api_v1_valuations_url, + params: { start_date: "04/30/2026" }, + headers: api_headers(@api_key) + assert_response :unprocessable_entity + + response_data = JSON.parse(response.body) + assert_equal "validation_failed", response_data["error"] + end + + test "should reject index with malformed account_id filter" do + get api_v1_valuations_url, + params: { account_id: "not-a-uuid" }, + headers: api_headers(@api_key) + assert_response :unprocessable_entity + + response_data = JSON.parse(response.body) + assert_equal "validation_failed", response_data["error"] + assert_equal "account_id must be a valid UUID", response_data["message"] + end + + test "should not expose internal index errors" do + Api::V1::ValuationsController.any_instance.stubs(:safe_page_param).raises(StandardError, "database password leaked") + + get api_v1_valuations_url, headers: api_headers(@api_key) + assert_response :internal_server_error + + response_data = JSON.parse(response.body) + assert_equal "internal_server_error", response_data["error"] + assert_equal "An unexpected error occurred", response_data["message"] + assert_not_includes response.body, "database password leaked" + end + + test "should reject index without API key" do + get api_v1_valuations_url + assert_response :unauthorized + end + # CREATE action tests test "should create valuation with valid parameters" do valuation_params = { @@ -207,6 +294,6 @@ class Api::V1::ValuationsControllerTest < ActionDispatch::IntegrationTest private def api_headers(api_key) - { "X-Api-Key" => api_key.display_key } + { "X-Api-Key" => api_key.plain_key } end end