Ideas to discuss about Statements API endpoints #1187
Conversation
| end | ||
|
|
||
| def index | ||
| @statements = Statements::FilterService.new(params).find_all |
There was a problem hiding this comment.
Ideally, the service should be unique, without making it specific for V1, V2...
There was a problem hiding this comment.
Final approach on services is to have separate folders with services per version
/services/api/v3/FilterService
It is pending the discussion around reusing code between API services.
| @@ -0,0 +1,6 @@ | |||
| module Statements | |||
| class FilterService | |||
| def self.find_all(params) | |||
There was a problem hiding this comment.
Services are not using the patter call, and embrance a single business purpose and a descriptive name on the main method: find_all
There was a problem hiding this comment.
Agreed on using similar naming to: https://github.com/DFE-Digital/early-careers-framework/blob/main/app/services/api/v3/finance/statements_query.rb (.statements, .statement)
There was a problem hiding this comment.
Why do we need to nest queries in /api though? Then we'll just end up with overlapping code when we want a statements query for the admin panel or any other place where we might want to find/list statements.
If only API stuff goes in the /api directory that's fine, but we should move away from that as an approach.
| @@ -0,0 +1,7 @@ | |||
| module Statements | |||
| class ShowService | |||
There was a problem hiding this comment.
We probably don't need a ShowService to just find a Statement, but added here to have a chat about it.
There was a problem hiding this comment.
I think we do because it might just do Statement.all today but tomorrow it might log, filter, sort, split into multiple smaller lists, and this provides a natural place to put that.
| @@ -1,4 +1,6 @@ | |||
| module API | |||
| class BaseController < ApplicationController | |||
| before_action :authenticate_request! | |||
There was a problem hiding this comment.
A couple of extractions on the BaseController as a reminder that we should have
most of this code written only once :)
| # Do we have a separate Spec for all endpoints? | ||
| # Do we have a separate file with the docs? statements/index_docs_spec.rb | ||
| # Do we embed the docs on each spec? statements/index_spec.rb | ||
| RSpec.describe 'Statements API', type: :request do |
There was a problem hiding this comment.
This is RSpec support for Swagger. Worth exploring other options...
There was a problem hiding this comment.
Team agrees in postponing this decision and look into previous spikes for swagger.
| @@ -0,0 +1,6 @@ | |||
| module Statements | |||
| class FilterService | |||
There was a problem hiding this comment.
It would be really good, if we decide to go with services, to describe what a service is.
| def show = head(:method_not_allowed) | ||
| def index = head(:method_not_allowed) | ||
| def show | ||
| @statement = Statement.find(params[:id]) |
There was a problem hiding this comment.
Preferred to use services, and multiple actions per service (find and find_all)
Statements::FilterService.find
Statements::FilterService.find_all| end | ||
|
|
||
| def index | ||
| @statements = Statements::FilterService.new(params).find_all |
There was a problem hiding this comment.
We prefer being explicit about parameters:
def index
Statements::FilterService.new(filter)
end
private
def filter
{
}
end| end | ||
|
|
||
| def index | ||
| @statements = Statements::FilterService.new(params).find_all |
There was a problem hiding this comment.
Final approach on services is to have separate folders with services per version
/services/api/v3/FilterService
It is pending the discussion around reusing code between API services.
| @@ -0,0 +1,6 @@ | |||
| module Statements | |||
| class FilterService | |||
| def self.find_all(params) | |||
There was a problem hiding this comment.
Agreed on using similar naming to: https://github.com/DFE-Digital/early-careers-framework/blob/main/app/services/api/v3/finance/statements_query.rb (.statements, .statement)
| json.array! @statements do |statement| | ||
| json.id statement.id | ||
| json.type 'statement' | ||
| json.attributes do |
There was a problem hiding this comment.
Team feels closer to serializers instead of jbuilder.
Developer picking up the card will show a few options for it.
| ### Open points | ||
| # Do we need controller tests? Or do we rely on Integration/Request tests | ||
| # Do we stub the service layer? Or do we rely on a real database request? | ||
| RSpec.describe API::V1::StatementsController, type: :controller do |
There was a problem hiding this comment.
Team agrees on not having controller tests.
There was a problem hiding this comment.
In favour of request specs? I added the examples specs in spec/controllers but they have type: :request.
The request/controller specs should only be ensuring that the submitted params are passed to the relevant service objects anyway, keeping business logic out of controllers is key here.
| # Open points: | ||
| # Are we able with using JBuilder for JSON generation | ||
| # | ||
| # Are we happy with doing light testing on the integration |
There was a problem hiding this comment.
We agree on light testing on the controller leaving edge and complex cases for services and serializers.
| xit 'only accepts json format' | ||
| xit 'returns 404 if statement does not exist' | ||
|
|
||
| describe 'JSON format' do |
There was a problem hiding this comment.
Team agrees on checking a few attributes (counts) just to make sure the serializer is hooked into the pipeline. Keep it very simple without duplicating tests that are already done in the serializer. Do not test values just the structure.
| # Do we have a separate Spec for all endpoints? | ||
| # Do we have a separate file with the docs? statements/index_docs_spec.rb | ||
| # Do we embed the docs on each spec? statements/index_spec.rb | ||
| RSpec.describe 'Statements API', type: :request do |
There was a problem hiding this comment.
Team agrees in postponing this decision and look into previous spikes for swagger.
|
Closing as it served its purpose |
Context
This PR collects ideas and discussions that we have had within the team with the
objective of agreeing in a number a approaches towards implementing the new
NPQ API.
This is the foundation for this PR.