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

frontsite: patron can cancel its own literature request #746

Merged
merged 2 commits into from Feb 25, 2020
Merged

frontsite: patron can cancel its own literature request #746

merged 2 commits into from Feb 25, 2020

Conversation

topless
Copy link
Member

@topless topless commented Feb 17, 2020

@topless topless self-assigned this Feb 17, 2020
@topless topless added this to the 2020/W06 milestone Feb 17, 2020
@topless topless force-pushed the 743-cancel-literature-request branch 2 times, most recently from c25ea1f to 99f5fcf Compare February 18, 2020 08:58
ntarocco
ntarocco reviewed Feb 18, 2020
View reviewed changes
Copy link
Contributor

@ntarocco ntarocco left a comment

Choose a reason for hiding this comment

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

Currently, I see that dreqid had update_permission_factory_imp=backoffice_permission, so the patron will not be able to update it.
Did you try to be logged in as the Patron that created the request?

@topless topless changed the title frontsite: patron can cancel its own literature request [WIP] frontsite: patron can cancel its own literature request Feb 18, 2020
@topless topless force-pushed the 743-cancel-literature-request branch 3 times, most recently from 9e29ce5 to 6564b0e Compare February 19, 2020 09:28
This was referenced Feb 19, 2020
Closed
Closed
@kpsherva kpsherva modified the milestones: 2020/W06, 2020/W08 Feb 19, 2020
@topless topless force-pushed the 743-cancel-literature-request branch 3 times, most recently from c0024f7 to 23acb2e Compare February 21, 2020 08:36
@topless topless changed the title [WIP] frontsite: patron can cancel its own literature request frontsite: patron can cancel its own literature request Feb 21, 2020
@topless topless changed the title frontsite: patron can cancel its own literature request [wip] frontsite: patron can cancel its own literature request Feb 21, 2020
@topless topless changed the title [wip] frontsite: patron can cancel its own literature request frontsite: patron can cancel its own literature request Feb 21, 2020
ntarocco
ntarocco reviewed Feb 21, 2020
View reviewed changes
Copy link
Contributor

@ntarocco ntarocco left a comment

Choose a reason for hiding this comment

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

It looks neat! Thanks a lot for the refactoring! I find it more clear to understand now. Good job!

invenio_app_ils/document_requests/loaders/jsonschemas/document_request_accept.py Outdated Show resolved Hide resolved
invenio_app_ils/document_requests/views.py Outdated Show resolved Hide resolved
invenio_app_ils/document_requests/loaders/jsonschemas/document_request_document.py Outdated

class Meta:
"""Meta attributes for the schema."""

unknown = EXCLUDE

remove_fields = fields.List(fields.Str())
document_pid = fields.Str()
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not mandatory because on delete there is no document_pid payload, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Exactly, if we want to be super conservative, in a previous commit, I was actually submitting the document_pid and I was checking if it matches the one we are trying to remove. We can go down either path wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is good enough! :)
I would maybe add a comment that on POST you have it, and on DELETE no.

Copy link
Contributor

Choose a reason for hiding this comment

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

@topless what about if you use partial? You can set it to required here, so that in the POST method it is automatically checked and you can get rid of your own validation, and in the DELETE, use partial=True.

Copy link
Member Author

Choose a reason for hiding this comment

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

@ntarocco I have made the document_pid required, and now I am submitting it also in the DELETE request. The main reason for that is that I didn't find a simple way to pass partial=True to the load method. As far I understood this is the place where the load() is called: https://github.com/inveniosoftware/invenio-records-rest/blob/526dcaef46dfe79acf737feee4570fd6e0b25679/invenio_records_rest/loaders/marshmallow.py#L91. According to the documentation for partial it should be something like `.load(request_json, partial=True). In invenio-records-rest we don't allow extra params to be passed down to load function. There are ways to fix/cheat this around, I am looking forward for you input cause I might have missed something in the process.

Copy link
Member Author

@topless topless Feb 24, 2020 •
edited

Choose a reason for hiding this comment

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

As it concerns, submitting data with DELETE there is a worth reading SO question https://stackoverflow.com/questions/299628/is-an-entity-body-allowed-for-an-http-delete-request, and a related comment on axios github axios/axios#897 (comment)

invenio_app_ils/document_requests/views.py Show resolved Hide resolved
invenio_app_ils/document_requests/views.py Outdated Show resolved Hide resolved
ui/src/authentication/services/AuthenticationService.js Show resolved Hide resolved
...t/DocumentRequestDetails/components/DocumentRequestSteps/components/ReviewStep/ReviewStep.js Outdated Show resolved Hide resolved
...pages/frontsite/PatronProfile/PatronCurrentDocumentRequests/PatronCurrentDocumentRequests.js Outdated Show resolved Hide resolved
ui/src/pages/frontsite/PatronProfile/PatronPendingLoans/PatronPendingLoans.js Show resolved Hide resolved
ui/src/pages/frontsite/PatronProfile/PatronPendingLoans/PatronPendingLoans.js Show resolved Hide resolved
ntarocco
ntarocco approved these changes Feb 21, 2020
View reviewed changes
Copy link
Contributor

@ntarocco ntarocco left a comment

Choose a reason for hiding this comment

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

LGTM! :)

kpsherva
kpsherva approved these changes Feb 24, 2020
View reviewed changes
ui/src/pages/frontsite/components/Document/DocumentLinks.js Outdated
@@ -21,7 +21,7 @@ export class DocumentLinks extends Component {

hasPermissions = eitem => {
if (!eitem.open_access) {
return sessionManager.authenticated;
return sessionManager.isAuthenticated();
Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for fixing this, I have no idea why it was working for me, I will check it

@topless topless force-pushed the 743-cancel-literature-request branch 3 times, most recently from 81f5c8b to b1b246b Compare February 24, 2020 15:52
@topless
document requests: makeover
7e8c0af 
- patron can cancel its own literature request
- restored circulation in resolved document
- rename onChangeDocument to onRemoveDocument
- restructured document request endpoints
- Document Request Reject need_record_permission
- confirmation modal when patron cancels literature request
- fix message for no literature requests
- cast patron_pid as a string when we fetch it
- refactored common helpers
- document request custom endpoints permission test
- rename the PendingRequestResource to be more explicit
- fix OrderSubmitForm serializer from inveniosoftware#738
- closes inveniosoftware#742 add the circulation field in the loan resolver in loan.py:document_resolver(loan_pid)
- closes inveniosoftware#743
@topless
document requests review
64a91ce 
- addressed review comments
- implemented isAuthenticated for session manager, to replace the property authenticated which was never defined
- fixed an issue with item link for loan when there is no item available
@topless topless force-pushed the 743-cancel-literature-request branch from b1b246b to 64a91ce Compare February 25, 2020 10:57
@kpsherva kpsherva merged commit defc369 into inveniosoftware:master Feb 25, 2020
@topless topless deleted the 743-cancel-literature-request branch February 25, 2020 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ntarocco ntarocco ntarocco approved these changes

@kpsherva kpsherva kpsherva approved these changes

Assignees

@topless topless

Labels
None yet
Projects
None yet
Milestone
2020/W08
Development

Successfully merging this pull request may close these issues.

patron cancel literature request
3 participants
@topless @ntarocco @kpsherva