Airlock: Creating an import/export request causes a routing error

[yahya130]

Describe the bug
Upon creating a new export/import request in Airlock, the URL route malforms and causes the UI to behave in an weird way. The issue is to do with routing when creating a new request. I have already forked the repo and fixed the issue. The issue to be exact is here:

AzureTRE/ui/app/src/components/shared/airlock/Airlock.tsx

Line 258 in 3196089

navigate(newRequest.id);

The path is being navigated from a relative point of view and just appended rather then re-written. The fix I put in the fork is the following:

    navigate(`../${newRequest.id}`, { relative: "path" });

Steps to reproduce

1. Open/create a new workspace and navigate to Airlock on the side panel.
2. Create a new import or export request.
3. Fill in the form and press create.

Expected:
The draft request to be created and the side panel to open automatically with the SAS URL information, with the URL /requests/{id}.

Actual:
The side panel closes and the path remains /requests/new/{id} but the side panel doesn't reopen with the SAS URL information. Double clicking requests simply appends the airlock id to the URL.

Work around:
Exit and enter back to the requests list view and double clicking from there allows the side panel to open.

Azure TRE release version: v0.15.2, UI version: 0.5.17, (but can also see the issue on main).

[marrobi]

Hmm, we had a similar issue here: #3821

This fixed the issue, but not sure of the root cause. Do you think this is a similar "issue"?

Are you able to do a pull request to fix the issue?

[yahya130]

@marrobi , it is similar in terms of being a routing issue but in a separate part of the code base. I done a scan of the code it looks like when this is fixed then there should be no more routing issues. In terms of the PR... happy to do it. In addition, I would suggest tests for the routing in the UI, would be super helpful as it is a critical part of the UI... currently I couldn't find any tests for it, correct me if I am wrong though.

[marrobi]

Correct, there are no UI tests at present. Be good to have some integrated with CI.

[yahya130]

Would be happy to create an issue for the routing tests too and implement them when I get the chance.

[yahya130]

@marrobi, I've taken a look at this issue further, it seems like the error actually happened because of the react-router-dom dependency, in the package.json file we reference the dependency react-router-dom version 6 and version 6 only:

AzureTRE/ui/app/package.json

Line 27 in 3196089

"react-router-dom": "6",

so when we run yarn install it generates a yarn.lock file and gets the latest sub version of 6. At the time of running it in our case it was version 6.20.0, when updating it to version 6.21.1 referenced in the main branch it works:

AzureTRE/ui/app/yarn.lock

Line 9403 in 3196089

react-router@6.21.1:

Since this version works, it would be appropriate to nail down version 6.21.1 in the package.json file so we don't introduce any other version in the future which causes a breaking change.

In addition, I still think updating the route in the react code to use more of an absolute path is valid, it is more secure, good practice and future proofs any version update of the dependency when it comes to routing.

Furthermore, taking a look at the package.json file it looks like all the dependencies update automatically when minor and patch changes happen, symbolized by the caret ^:

AzureTRE/ui/app/package.json

Lines 5 to 30 in 3196089

	"dependencies": {
	"@azure/msal-browser": "^2.35.0",
	"@azure/msal-react": "^1.5.12",
	"@fluentui/react": "^8.114.1",
	"@fluentui/react-file-type-icons": "^8.7.9",
	"@reduxjs/toolkit": "^1.8.6",
	"@rjsf/core": "^4.2.3",
	"@rjsf/fluent-ui": "^4.2.3",
	"@testing-library/jest-dom": "^6.2.0",
	"@testing-library/react": "^14.0.0",
	"@testing-library/user-event": "^14.4.3",
	"@testing-library/dom": "^7.21.4",
	"@types/jest": "^29.5.0",
	"@types/node": "^20.4.2",
	"@types/react": "^18.0.25",
	"@types/react-dom": "^18.2.6",
	"moment": "^2.29.4",
	"node-sass": "^8.0.0",
	"react": "^18.2.0",
	"react-dom": "^18.2.0",
	"react-markdown": "^8.0.3",
	"react-redux": "^8.0.4",
	"react-router-dom": "6",
	"remark-gfm": "^3.0.1",
	"typescript": "^5.1.6",
	"web-vitals": "^3.3.0"

It would be ideal to remove all forms of automatically updating packages to prevent breaking changes, but that would be a separate issue/ticket and I would be sceptical to do this for all packages until there are UI tests in place. For now I'd advise caution when yarn install is used in the automated script or locally:

AzureTRE/devops/scripts/build_deploy_ui.sh

Line 26 in 3196089

yarn install

[marrobi]

Can you do PR to pin the packages and make the routes more robust?

Thanks.

[yahya130]

@marrobi Sure thing, I have the branch ready locally, just needed permissions from your side to push my branch upstream and create PR :)

[martinpeck]

You can PR from your fork/branch, can you?

[martinpeck]

@yahya130 if you need help with forks/pushing the PR etc. feel free to shout

[damoodamoo]

This makes a lot of sense - I think we've seen 2 issues relating to the same thing with the router now. Suspect that a minor version bump changed navigate() from defaulting to /{route} to just {route}.

Agree with @yahya130 - let's pin the version and also force the /.

Great catch, thank you!

[jjgriff93]

Great find and agree with the above!