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

Expose withdrawal of previously submitted proposal by PI #288

Closed
DJWalker42 opened this issue Dec 13, 2024 · 8 comments · Fixed by #293
Closed

Expose withdrawal of previously submitted proposal by PI #288

DJWalker42 opened this issue Dec 13, 2024 · 8 comments · Fixed by #293
Assignees
Labels
Blocked is blocked by other issues core functionality A requirement of the tool enhancement New feature or request

Comments

@DJWalker42
Copy link
Contributor

Add a button to allow a PI to withdraw a previously submitted proposal, most natural place for this is on the home/landing page for the PI user, alongside the list of submitted proposals.

@DJWalker42 DJWalker42 added enhancement New feature or request core functionality A requirement of the tool labels Dec 13, 2024
@DJWalker42 DJWalker42 self-assigned this Dec 16, 2024
@DJWalker42 DJWalker42 added the Blocked is blocked by other issues label Dec 16, 2024
@DJWalker42
Copy link
Contributor Author

I've added a "Withdraw" action button to the list of cycles to which a proposal has been submitted on the Editor landing page.

Blocked:
Currently, doesn't work claiming a '401, unauthorised'. This can be overcome by removing the "@RolesAllowed" annotation on the API call (which is subverting the semantics). However, even after doing this, in the implementation of the call the Person currentUser is null, meaning it cannot check the "InvestigatorKind", and you are falsely identified as not the PI for the proposal. Unsure how to proceed.

@AllanEngland
Copy link
Contributor

For some reason the API is not getting a bearer token in the withdrawal request from the GUI, I'll keep investigating.

@AllanEngland
Copy link
Contributor

My current belief is this fails because of how it is implemented in React. I suspect the bearer token is not populated where it would break React's rules of hooks, but without a visible error I can't be completely sure.

As a work around, I've explicitly added the token to the request and it now works. To progress it from here, I'll add more authorisation checks to API endpoints and see if there's commonality where they fail.

@pahjbo
Copy link
Member

pahjbo commented Jan 9, 2025

Looking at this in detail might well provide some hints as to why #144 is a problem

@AllanEngland
Copy link
Contributor

So, after far more tinkering and experimentation I realised the authentication header is not sent with (auto generated) fetchXYZ functions.

It's quite clear when you follow the code chain but, for some reason, I hadn't noticed until now. It turns out the useXYZ functions wrap their equivalent fetchXYZ and add in fetcherOptions containing the authentication header.

Exchanging the fetchXXY calls for the equivalent useXYZ calls is not always possible because of the React rules of hooks.

A decent solution is to change the openapi-codegen library so it adds the authentication header to all fetchXYZ functions. Hopefully the issue with content type can be fixed too

I'll start looking into this, but if an interim fix is required there’s a relatively simple way to make these functions work.

Create a variable in scope where a React hook is allowed, such as a parent function or element.
e.g.
const fetcherOptions = useProposalToolContext().fetcherOptions;

Then expand this variable into the fetcher parameters
e.g.
fetchUserProposalsSubmittedWithdrawProposal({ ...fetcherOptions, pathParams: {submittedProposalId: submissionDetail?.submittedProposalId!}, queryParams: {cycleId: props.cycle.dbid!} })

However, if the fetch needs to specify a different header, remember to merge this rather than replace as above

e.g.
headers: {...fetcherOptions.headers, "Content-Type": "text/plain"}

I don't know if any of this is connected to #144 though, any thoughts?

@AllanEngland
Copy link
Contributor

Or, if there's another way to manage global variables, we could change the way the authentication bearer token is stored and applied.

@AllanEngland
Copy link
Contributor

I've implemented the workaround in branch ARE-patch-fetch-operations and tagged everything with HACK #1 (easily changed if anyone has a better suggestion!)

I've also tested a good bit and it appears to work with the auth bearer token sent on all api calls. Testing the GUI isn't automated though, so very happy for others to have a go before merging into master.

There's a chance this change introduces new issues that I haven't spotted yet, and I don't know what will happen if a token is updated between a page render and a fetch request.

@DJWalker42
Copy link
Contributor Author

This has been done so closing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Blocked is blocked by other issues core functionality A requirement of the tool enhancement New feature or request
Projects
None yet
3 participants