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.

Sign up for GitHub

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

Jump to bottom

loan: update request pickup location #935

Merged
merged 1 commit into from Apr 28, 2020
Merged

loan: update request pickup location #935

merged 1 commit into from Apr 28, 2020

Conversation

AoNoOokami
Copy link
Contributor

@AoNoOokami AoNoOokami commented Apr 15, 2020 •
edited

  • Adds route to update request pickup location.

Co-Authored-by: Alicia Zangger alicia.zangger@rero.ch

Why are you opening this PR?

https://tree.taiga.io/project/rero21-reroils/task/1438?kanban-status=1224895
Task 1438 of US 1355 - cancel and edit a request

How to test?

Please do code review. It should be tested with rero/rero-ils-ui#229

Code review check list

  • Commit message template compliance.
  • Commit message without typos.
  • File names.
  • Functions names.
  • Functions docstrings.
  • Unnecessary commited files?
  • Extracted translations?

@AoNoOokami AoNoOokami self-assigned this Apr 15, 2020
@rerowep
Copy link
Contributor

rerowep commented Apr 17, 2020

It is also good to get the coverage to green if possible.

@AoNoOokami AoNoOokami added the WIP label Apr 20, 2020
@AoNoOokami AoNoOokami force-pushed the zaa-#1438-update-loan branch 7 times, most recently from d04540d to 6c05a06 Compare April 24, 2020 09:16
@AoNoOokami AoNoOokami changed the title loan: update loan loan: update request pickup location Apr 24, 2020
@AoNoOokami AoNoOokami removed the WIP label Apr 24, 2020
@AoNoOokami AoNoOokami marked this pull request as ready for review April 24, 2020 09:30
@AoNoOokami AoNoOokami mentioned this pull request Apr 24, 2020
Merged
7 tasks
@AoNoOokami AoNoOokami added this to the release: v0.8.0 milestone Apr 24, 2020
BadrAly
BadrAly suggested changes Apr 26, 2020
View reviewed changes
rero_ils/modules/items/api_views.py Outdated Show resolved Hide resolved
@AoNoOokami AoNoOokami requested review from BadrAly and jma and removed request for rerowep April 27, 2020 07:46
jma
jma requested changes Apr 27, 2020
View reviewed changes
rero_ils/modules/items/api_views.py Outdated Show resolved Hide resolved
rero_ils/modules/items/api_views.py
loan = Loan.get_record_by_pid(loan_pid)
loan['pickup_location_pid'] = pickup_location_pid
if not loan.get('state') == 'PENDING':
return jsonify({'status': 'error: Forbidden'}), 403
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be a bad request not a forbidden

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If loan state is not pending, updating pickup location is not allowed. That's why I use a 403 error message as response, because I check that the state chart is followed. Later, more complex cases will be managed and this part of the function will be modified.
https://en.wikipedia.org/wiki/List_of_HTTP_status_codes#4xx_Client_errors

Copy link
Contributor

Choose a reason for hiding this comment

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

If loan state is not pending, updating pickup location is not allowed. That's why I use a 403 error message as response, because I check that the state chart is followed. Later, more complex cases will be managed and this part of the function will be modified.

You are right.

loan: update request pickup location
bb976aa 
* Adds route to update request pickup location.

Co-Authored-by: Alicia Zangger <alicia.zangger@rero.ch>
@AoNoOokami AoNoOokami merged commit 28b731f into rero:dev Apr 28, 2020
@AoNoOokami AoNoOokami deleted the zaa-#1438-update-loan branch June 2, 2020 09:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rerowep rerowep rerowep left review comments

@jma jma jma approved these changes

@zannkukai zannkukai zannkukai approved these changes

@benerken benerken benerken approved these changes

@BadrAly BadrAly BadrAly approved these changes

Assignees

@AoNoOokami AoNoOokami

Labels
None yet
Projects
None yet
Milestone
release: v0.8.0
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@AoNoOokami @rerowep @jma @zannkukai @benerken @BadrAly