UIIN-2784: Set central tenant id in the request when Member tenant deletes a shared record

[OleksandrHladchenko1]

Purpose

• If user try to delete shared record from Member tenant, error message appeared. In DevTools he will see 404 error. The task is to remove this errror.

Approach

• When member tenant is setting shared instance for deletion, pass central tenant id in the request.
• Created separate function setRecordForDeletion which performs setting record for deletion using dynamically passed tenantId
• Updated unit tests

Refs

UIIN-2784

Screenshots

UIIN-2784.mp4

[github-actions]

Jest Unit Test Statistics

    1 files  ±0  230 suites  ±0   15m 51s ⏱️ +3s
931 tests +2  929 ✔️ +2  2 💤 ±0  0 ❌ ±0 
937 runs  +2  935 ✔️ +2  2 💤 ±0  0 ❌ ±0 

Results for commit 3f220a3. ± Comparison against base commit 1477513.

♻️ This comment has been updated with latest results.

[zburke]

I have a few questions:

1. Rather than directly calling fetch(), what about wrapping ViewInstance with withOkapiKy and passing okapiKy to setRecordForDeletion()? That will let you avoid a lot of the boilerplate of x-okapi-this and x-okapi-that. **
2. What's the value of wrapping the fetch in try/catch and throwing given you just catch and return the error? If you want to return the Error, just return it. Feels like a weird pattern though, since it doesn't alert the calling function that anything has gone wrong unless it inspects the type of the returned value, which it doesn't.

** OK, it's not quite that easy since we missed updating withOkapiKy in folio-org/stripes-core#1348 / STCOR-747 when you added the tenant prop to useOkapiKy. We should add it now so withOkapiKy and useOkapiKy have parity, agreed?

[OleksandrHladchenko1]

I have a few questions:

1. Rather than directly calling fetch(), what about wrapping ViewInstance with withOkapiKy and passing okapiKy to setRecordForDeletion()? That will let you avoid a lot of the boilerplate of x-okapi-this and x-okapi-that. **
2. What's the value of wrapping the fetch in try/catch and throwing given you just catch and return the error? If you want to return the Error, just return it. Feels like a weird pattern though, since it doesn't alert the calling function that anything has gone wrong unless it inspects the type of the returned value, which it doesn't.

** OK, it's not quite that easy since we missed updating withOkapiKy in folio-org/stripes-core#1348 / STCOR-747 when you added the tenant prop to useOkapiKy. We should add it now so withOkapiKy and useOkapiKy have parity, agreed?

Hi @zburke
I fixed the second part. As for the first one, I totally agree that we need to make changes in withOkapiKy, but there is not so much time before the release to make this changes. What about leaving the implementation using fetch as is and create a ticket for making chenges in withOkapiKy and plan it for Ramsons. What do you think?

[mariia-aloshyna]

@OleksandrHladchenko1 add handling of an error, please. Show unsuccessful toast for the case when the request failed or any error occurred

[zburke]

This is different but I'm not sure it's better. The calling code does fx().then().catch(). Previously, your fx() implementation included try/catch, but caught and returned the error, so the parent catch block would never be invoked. Now you removed the try/catch, but also removed its throw, so again the parent catch block will never be invoked.

Am I misunderstanding something here? IIUC, the bug report identified a 404 that was causing an error toast. Sending the correct (central) x-okapi-tenant in the request header fixes the 404 (good!) ... but I think we should keep the error-handling in place in case of some other problem with the request. Agreed?

[zburke]

These changes LGTM now. @mariia-aloshyna , do you agree that we have resolved both problems?

1. correctly set the central tenant id in the request
2. continue to provide error handling in case the request fails

[sonarcloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
88.9% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud