Restructure Airlock requests & add createdBy field

[jjgriff93]

Resolves #2765

What is being addressed

This PR fixes the querying of airlock requests by the initiator of the request by adding a createdBy field to the requests as well as reworking several fields to be more descriptive

How is this addressed

• added a createdBy field to keep the original request creator value
• updated Api queries to use this field
• renamed some airlock properties to more descriptive and sensible values, such as user to updatedBy (both in the root and the history objects)
• De-coupled the airlock requests model in the UI from the base resource model to reflect the structure in the API models
• Added DB migration to fill in the createdBy value from the history if present and also rename the other fields to their new values
• Also corrected some things that have been bothering me such as consistent snake casing and camel casing where appropriate

[github-actions]

Unit Test Results

518 tests   518 ✔️  18s ⏱️
    1 suites      0 💤
    1 files        0 ❌

Results for commit 0e594f5.

♻️ This comment has been updated with latest results.

[jjgriff93]

/test

[github-actions]

🤖 pr-bot 🤖

🏃 Running tests: https://github.com/microsoft/AzureTRE/actions/runs/3313797214 (with refid 135c012a)

(in response to this comment from @jjgriff93)

[damoodamoo]

I understand the approach but if the longer term plan is to add an initiator field - shall we not just do that now?

I'm also nervous about coupling any logic to the History array as we've previously discussed moving that out to a separate Cosmos collection, so this would have to be updated in that case.

[jjgriff93]

I understand the approach but if the longer term plan is to add an initiator field - shall we not just do that now?

I'm also nervous about coupling any logic to the History array as we've previously discussed moving that out to a separate Cosmos collection, so this would have to be updated in that case.

I don't think there's any plan to add an initiator field to all resource objects - what I discussed with Stuart was the potential of all resources having the field renamed from user to last_updated_by to avoid any confusion, but that's something that needs to be discussed as a bigger group as it affects the whole TRE API

On the history array front yes that's true - the logic could be updated at a later date to fetch the history item but would involve a second call to the db by the API (although I imagine it would need to do this anyway)

@damoodamoo Happy to go down the initiator field route if you think that backwards compatibility isn't an issue? i.e. requests created before this won't have an initiator field

[damoodamoo]

Not sure why we'd need to update all Resource types - as the Airlock Request is it's own model? The shape of it is very similar to a Resource but it's defined separately and lives in a separate collection. Maybe we'd want to update resource types at some point but I don't think we'd need to do that in this work. What do you think about just adding the field we need to the airlock request model?

For existing requests we could add a migration which would move the top history item user to the initiator field - or we could not worry about it as I can't imagine there are many inflight requests that we need to worry about at this stage? @marrobi ?

[jjgriff93]

Not sure why we'd need to update all Resource types - as the Airlock Request is it's own model? The shape of it is very similar to a Resource but it's defined separately and lives in a separate collection. Maybe we'd want to update resource types at some point but I don't think we'd need to do that in this work. What do you think about just adding the field we need to the airlock request model?

For existing requests we could add a migration which would move the top history item user to the initiator field - or we could not worry about it as I can't imagine there are many inflight requests that we need to worry about at this stage? @marrobi ?

It's an extension of the Resource model (in the UI models) so inherits user from that - however discussed this on the standup this morning and have decided to go ahead and add the additional initiator field to the AirlockRequest model. Oxford haven't raised any concerns over any previous requests missing this field and I think we're early enough along for this not to be a problem - and like you said it then covers us for any changes to the history model at a later date. Also means later on should we decide to we can adopt the initiator field into the core resource model if we're moving the history out and want quick access to the person who created the resource, so this will act as a nice test case for this

[jjgriff93]

Have reworked this as per our discussions:

• added a createdBy field to keep the original request creator value
• updated Api queries to use this field
• renamed some airlock properties to more descriptive and sensible values, such as user to updatedBy (both in the root and the history objects)
• De-coupled the airlock requests model in the UI from the base resource model to reflect the structure in the API models
• Added DB migration to fill in the createdBy value from the history if present and also rename the other fields to their new values
• Also corrected some things that have been bothering me such as consistent snake casing and camel casing where appropriate

[jjgriff93]

/test

[github-actions]

🤖 pr-bot 🤖

🏃 Running tests: https://github.com/microsoft/AzureTRE/actions/runs/3322722949 (with refid 135c012a)

(in response to this comment from @jjgriff93)

[jjgriff93]

/test

[github-actions]

🤖 pr-bot 🤖

🏃 Running tests: https://github.com/microsoft/AzureTRE/actions/runs/3324592277 (with refid 135c012a)

(in response to this comment from @jjgriff93)

[damoodamoo]

looks great + a much better shape of the object :)

[jjgriff93]

/test

[github-actions]

🤖 pr-bot 🤖

🏃 Running tests: https://github.com/microsoft/AzureTRE/actions/runs/3329913926 (with refid 135c012a)

(in response to this comment from @jjgriff93)