search

inveniosoftware / invenio-circulation

Library circulation module for Invenio.
https://invenio-circulation.readthedocs.io
MIT License
0 stars 18 forks source link

views: fix actions blueprint for multiple entries #47

Closed jma closed 6 years ago

jma commented 6 years ago

Signed-off-by: Johnny Mariéthoz Johnny.Mariethoz@rero.ch

zzacharo commented 6 years ago

@jma https://github.com/inveniosoftware/invenio-circulation/pull/47/files#diff-2b66d1d23d98cd75cd5ae11e29cb6183L117 good catch thanks!

zzacharo commented 6 years ago

@jma I am thinking that the probably we should put the LoanActions view only in loan_pid rest. If you extend the endpoints probably it doesn't make sense to register actions there. wdyt?

jma commented 6 years ago

@zzacharo yes you are right. In such case we need to select the right option. I will try to solve it.

jma commented 6 years ago

@zzacharo I think it is solved.

zzacharo commented 6 years ago

@jma I think that we should just hardcode it inside the build method. It's a bit confusing and actually we don't gain something from having it as _CIRCULATION_LOAN_PID_TYPE. WDYT?

jma commented 6 years ago

@zzacharo why using it api.py and not in views.py?

zzacharo commented 6 years ago

@jma but it's more views related not api.

zzacharo commented 6 years ago

@jma also can you rebase? When you do it let me know and we can merge it.

zzacharo commented 6 years ago

@jma I messed up your pr with another merge commit. I tried to update the branch from here. Sorry for that! Can you just rebase and push force in your pr again?

zzacharo commented 6 years ago

My only comment is if you want to hardcode the value loan_pid in the build_blueprint_with_loan_actions instead of the config. Other than that I am ok with this PR to be merged.