Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(request): update a request #93

Merged
merged 5 commits into from
Dec 20, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion app/controllers/api/v1/requests_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,15 @@ def create
end
end

def update
plant_attributes_from_params(@request)
if @request.update(request_params)
Comment on lines +53 to +54
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

En mettant les instructions dans cet ordre, on peut s'exposer à un petit souci de cohérence.
La première méthode appelée, plant_attributes_from_params, regarde si un ID est passé en paramètre, et assigne les champs plant_stage_name et plant_name si c'est le cas.

Puis, la méthode update assigne tous les paramètres passés par l'utilisateur ; dont les fameux plant_stage_name et plant_name.

Ce qui veut dire que dans l'idée, la méthode plant_attributes_from_params peut ici être overridée par les paramètres passés par le front, et donc créer des valeurs "illogiques" (par exemple, si passe l'id d'une rose mais je mets en dur que le nom est une tulipe ; et ça sera enregistré tel quel dans la bdd).

En inversant l'ordre d'appel des méthodes, on évite cette potentielle "faille" :)

Suggested change
plant_attributes_from_params(@request)
if @request.update(request_params)
@request.assign_attributes(request_params)
plant_attributes_from_params(@request)
if @request.save

(si tu regardes bien, c'est d'ailleurs bien dans cet ordre que sont appelées les méthodes dans le create, ou qu'elles l'étaient dans la méthode update d'origine, qui a récemment été supprimée dans une autre PR)

render json: @request.to_blueprint, status: :ok
else
render_validation_error(@request)
end
end

def accept
if @request.fire_state_event(:accept)
render json: @request.to_blueprint
Expand Down Expand Up @@ -98,7 +107,7 @@ def set_request

def request_params
params.permit(
:plant_stage_id, :name, :plant_name, :plant_stage_name, :quantity, :due_date,
:plant_stage_id, :plant_name, :plant_stage_name, :name, :quantity, :due_date,
:comment, :temperature, :photoperiod, :requester_first_name, :requester_last_name,
:requester_email, :laboratory
)
Expand Down
9 changes: 9 additions & 0 deletions app/policies/request_policy.rb
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,15 @@ def complete?
true
end

def update?
if record.request_distributions.exists?
record.errors.add(:base, 'cannot update a request with associated request distributions')
false
else
true
end
end
Comment on lines +32 to +39
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok pour la logique 👌 si on veut l'écrire de manière un peu plus "rails-ish" et concise, ça pourrait donner quelque chose comme ça :

Suggested change
def update?
if record.request_distributions.exists?
record.errors.add(:base, 'cannot update a request with associated request distributions')
false
else
true
end
end
def update?
return true unless record.request_distributions.exists?
record.errors.add(:base, 'cannot update a request with associated request distributions')
false
end


def destroy?
if record.pending?
true
Expand Down
2 changes: 1 addition & 1 deletion config/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@

resources :plants, only: %i[index show create update destroy]

resources :requests, only: %i[index show create destroy], shallow: true do
resources :requests, only: %i[index show create update destroy], shallow: true do
get :requests_to_handle_count, on: :collection
post :accept, on: :member
post :refuse, on: :member
Expand Down
4 changes: 3 additions & 1 deletion db/seeds.rb
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,9 @@
plant_stage: Plant.last.plant_stages.last,
comment: Faker::Movies::LordOfTheRings.quote,
due_date: Date.current + 2.months,
quantity: 200
quantity: 200,
temperature: 'Froid',
photoperiod: 16
)

# RequestDistributions
Expand Down
2 changes: 0 additions & 2 deletions spec/acceptance/api/v1/me_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,9 @@
put '/api/v1/me' do
parameter :first_name, 'The new first name', with_example: true
parameter :last_name, 'The new last name', with_example: true
parameter :laboratory, 'The new laboratory (only for a requester)', with_example: true

let(:first_name) { 'Updated first name' }
let(:last_name) { 'Updated last name' }
let(:laboratory) { 'Updated laboratory' }

let(:raw_post) { params.to_json }

Expand Down
58 changes: 55 additions & 3 deletions spec/acceptance/api/v1/requests_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -192,9 +192,7 @@
request.update(status: :pending)
end

example 'Cancel a request' \
'If the request is already accepted and the current user is a requester, ' \
'the request will be set as `in_cancelation`' do
example 'Cancel a request' do
authentication :basic, "Bearer #{user_token.token}"

do_request
Expand Down Expand Up @@ -238,4 +236,58 @@
expect { request.reload }.to raise_exception(ActiveRecord::RecordNotFound)
end
end

put '/api/v1/requests/:id' do
let!(:request3) { requests(:request3) }
let(:id) { request3.id }

parameter :plant_stage_id, 'ID of the requested plant_stage', with_example: true
parameter :name, 'Name of the request', with_example: true
parameter :plant_name,
'Name of the requested plant (ignored if plant_stage_id given)',
with_example: true
parameter :plant_stage_name,
'Name of the requested plant stage (ignored if plant_stage_id given)',
with_example: true
parameter :due_date, 'Due date of the request', with_example: true, type: :date
parameter :quantity, 'Quantity of plants desired', with_example: true, type: :integer
parameter :temperature,
'(Optional) Temperature of cultivation desired',
with_example: true,
type: :integer
parameter :photoperiod,
'(Optional) Photoperiod of cultivation desired (in hour/day)',
with_example: true,
type: :integer

let(:plant_stage_id) { Plant.last.plant_stages.last.id }
let(:name) { 'My request' }
let(:plant_name) { Plant.last.name }
let(:plant_stage_name) { Plant.last.plant_stages.last.name }
let(:due_date) { Date.current + 6.months }
let(:quantity) { 150 }
let(:temperature) { 'Froid' }
let(:photoperiod) { 12 }

let(:raw_post) { params.to_json }

example 'Update a request' do
authentication :basic, "Bearer #{user_token.token}"
do_request

expect(status).to eq(200)

request.reload
Comment on lines +279 to +280
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

vu qu'ici on n'utilise pas l'objet request plus bas, on n'avait pas spécialement besoin de le reload :)

Suggested change
request.reload


response = JSON.parse(response_body)
expect(response['plant_stage_id']).to eq(plant_stage_id)
expect(response['plant_stage_name']).to eq(plant_stage_name)
expect(response['plant_name']).to eq(plant_name)
expect(response['name']).to eq(name)
expect(response['due_date']).to eq(due_date.strftime('%F'))
expect(response['quantity']).to eq(quantity)
expect(response['temperature']).to eq(temperature)
expect(response['photoperiod']).to eq(photoperiod)
end
end
end
5 changes: 1 addition & 4 deletions spec/config/shared_contexts.rb
Original file line number Diff line number Diff line change
@@ -1,12 +1,9 @@
# frozen_string_literal: true

RSpec.shared_context 'with authenticated requester' do
RSpec.shared_context 'without authentication' do
let(:user) { users(:user1) }
let(:application) { oauth_applications(:application) }
let(:token) { Doorkeeper::AccessToken.create!(resource_owner_id: user.id, application:) }
let(:headers) do
{ Authorization: "Bearer #{token.token}" }
end
end

RSpec.shared_context 'with authenticated grower' do
Expand Down
106 changes: 99 additions & 7 deletions spec/requests/api/v1/requests_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,8 @@
expect(response.parsed_body.count).to eq(2)
expect(response.headers['Pagination-Current-Page']).to eq(1)
expect(response.headers['Pagination-Per']).to eq(2)
expect(response.headers['Pagination-Total-Pages']).to eq(1)
expect(response.headers['Pagination-Total-Count']).to eq(2)
expect(response.headers['Pagination-Total-Pages']).to eq(2)
expect(response.headers['Pagination-Total-Count']).to eq(3)
end

it 'gets requests filtered by status' do
Expand Down Expand Up @@ -191,7 +191,7 @@
end
end

it_behaves_like 'with authenticated requester' do
it_behaves_like 'without authentication' do
it 'can create a request from an existing plant_stage' do
plant = Plant.last
plant_stage = plant.plant_stages.last
Expand Down Expand Up @@ -403,13 +403,12 @@
end
end

context 'when 404' do
it_behaves_like 'with authenticated requester' do
context 'when 401' do
it_behaves_like 'without authentication' do
it 'can\'t delete a request' do
delete("/api/v1/requests/#{id}", headers:)

expect(status).to eq(403)
expect(response.parsed_body.dig('error', 'message')).not_to be_blank
expect(status).to eq(401)
end
end
end
Expand All @@ -429,4 +428,97 @@
end
end
end

describe 'PUT api/v1/requests/:id' do
context 'when 403' do
it_behaves_like 'with authenticated grower' do
it 'can\'t update a request containing request_distributions' do
put(
"/api/v1/requests/#{id}",
headers:,
params: {
name: 'My request',
quantity: 150,
due_date: Date.current + 6.months,
comment: 'My comment',
temperature: 'Chaud',
photoperiod: 4
}
)

expect(status).to eq(403)
expect(response.parsed_body.dig('error', 'message')).not_to be_blank
end
end
end

context 'when 422' do
let!(:request3) { requests(:request3) }
let!(:id) { request3.id }

it_behaves_like 'with authenticated grower' do
it 'fails to update a request' do
allow_any_instance_of(Request).to receive(:update).and_return(false)

put(
"/api/v1/requests/#{id}",
headers:,
params: {
name: 'My request',
quantity: 150,
due_date: Date.current + 6.months,
comment: 'My comment',
temperature: 'Chaud',
photoperiod: 4
}
)

expect(status).to eq(422)
expect(response.parsed_body.dig('error', 'message')).not_to be_blank
end
end
end

context 'when 404' do
it_behaves_like 'with authenticated grower' do
it 'can\'t update a request that doesn\'t exist' do
put(
'/api/v1/requests/999',
headers:,
params: {
name: 'My request',
quantity: 150,
due_date: Date.current + 6.months,
comment: 'My comment',
temperature: 'Chaud',
photoperiod: 4
}
)

expect(status).to eq(404)
expect(response.parsed_body.dig('error', 'message')).not_to be_blank
end
end
end

context 'when 401' do
it_behaves_like 'without authentication' do
it 'can\'t update a request' do
put(
"/api/v1/requests/#{id}",
headers:,
params: {
name: 'My request',
quantity: 150,
due_date: Date.current + 6.months,
comment: 'My comment',
temperature: 'Chaud',
photoperiod: 4
}
)
expect(status).to eq(401)
end
end
end
end
end
27 changes: 27 additions & 0 deletions test/fixtures/requests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,33 @@ request2:
zone: *2
time: *3

request3:
id: 3
requester_first_name: Alice
requester_last_name: Johnson
requester_email: [email protected]
laboratory: Research Lab
handler_id: 3
plant_stage_id: 5
name: Research request
plant_name: Lettuce
plant_stage_name: germination
status: pending
comment: This is a request for research purposes.
due_date: 2021-06-15
quantity: 50
temperature: "Cool"
photoperiod: 12
created_at: !ruby/object:ActiveSupport::TimeWithZone
utc: &1 2021-01-20 14:35:08.434029000 Z
zone: &2 !ruby/object:ActiveSupport::TimeZone
name: Etc/UTC
time: *1
updated_at: !ruby/object:ActiveSupport::TimeWithZone
utc: &3 2021-01-20 14:35:08.434029000 Z
zone: *2
time: *3

# == Schema Information
#
# Table name: requests
Expand Down
Loading