Fix selection errors #1210

[louise-davies]

Description

Requires a build of specific icatproject/topcat#483/ral-facilities/datagateway-download-api#12 branches.

This PR changes the multi-remove useRemoveFromCart method to use the same endpoint as useAddToCart with the remove parameter. I also added 431 retries to all TopCAT useMutation uses (as useMutation doesn't automatically retry 3 times like useQuery does). This should hopefully remove all errors.

I didn't bother changing the remove from cart methods in datagateway-download as they either supply a single ID or an asterisk and so doesn't have the URL length problem.

Testing instructions

Add a set up instructions describing how the reviewer should test the code

• Review code
• Check Actions build
• Review changes to test coverage
• I will be deploying this change and the relevant TopCAT snapshot to DLS preprod so it can be fully tested.

Agile board tracking

Closes #1210

[codecov]

Codecov Report

Merging #1231 (35e91e7) into develop (400305b) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff            @@
##           develop    #1231   +/-   ##
========================================
  Coverage    97.48%   97.49%           
========================================
  Files          134      134           
  Lines         6805     6827   +22     
  Branches      2046     2055    +9     
========================================
+ Hits          6634     6656   +22     
  Misses         155      155           
  Partials        16       16           

Flag	Coverage Δ
common	98.16% <100.00%> (+<0.01%)	⬆️
dataview	97.98% <ø> (ø)
download	94.96% <100.00%> (+0.07%)	⬆️
search	97.37% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
packages/datagateway-common/src/api/cart.tsx	100.00% <100.00%> (ø)
packages/datagateway-download/src/downloadApi.ts	95.58% <100.00%> (+0.16%)	⬆️
...kages/datagateway-download/src/downloadApiHooks.ts	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 400305b...35e91e7. Read the comment docs.

[louise-davies]

Note - the e2e tests will fail on CI until we release the relevant version of TopCAT to deploy on CI

[sam-glendenning]

Works great! Good use of retry to give the request another go in the unlikely event it fails

[sam-glendenning]

Quick final comment: are we still using the removeFromCart function in cart.tsx? Or can we remove it?

[louise-davies]

I'm pretty sure it's exported and used in datagateway-download as when I refactored download I tried to reuse any base axios request functions to use in the new react-query hooks, but I can move it to download if you think that's a better place for it now. As I mentioned in the PR description I still use the DELETE method in download as it's arguably more descriptive and there's no multi-deselect in download.

[sam-glendenning]

Ah yes I see, it's used in the useRemoveEntityFromCart hook, my bad. In my opinion, removeFromCart probably doesn't belong in cart.tsx anymore, I think it's best we move it to downloadApi or something similar.

[sam-glendenning]

Just a couple of things I wanted to ask, since I've forgotten some of the context around this fix:

1. Error code 431 is quite specific, were we always seeing this code and is it always this specific number?
2. Apologies if I'm missing something but error code 431 corresponds with "The server is unwilling to process the request because either an individual header field, or all the header fields collectively, are too large". If we retry the same request when getting this error, why would it work a second time?

[louise-davies]

@sam-glendenning this is an intermittent error that TopCAT sometimes gives - the only way to fix it was to redeploy TopCAT but in the meantime, if you retried any request then it might go through within 3 tries. The same request could fail with 431 and then go through when you retry it. It is always 431 and we don't know why TopCAT was responding like that. It's only significant in that useMutation by default doesn't retry requests at all (due to them being used to mutate things on the server I would guess) - so get requests to TopCAT would be fine as the 431 responses would trigger retries but the useMutations would always fail.

[sam-glendenning]

Thanks for the clarification! Happy with this, works well after testing on Diamond preprod