DPL-1052 Research - Limit edit access for labware Retention Instruction metadata

[SujitDey2022]

User story This story is the predecessor to #3676 and is needed to be done as a research story as there may be a need for a change in how and where the metadata is stored - because custom metadata is a key-value pair store, and the user needs to limit edit access to just the labware retention instructions and not ALL other metadata. - https://github.com/sanger/sequencescape/issues/3676#issuecomment-1882735216

Who are the primary contacts for this story Katy T. Danni W.

Who is the nominated tester for UAT Katy T. Andrew S.

Acceptance criteria To be considered successful this research story needs to investigate the solution approach for implementing the following requirements,

• [x] Limit edit access for labware retention instructions to only the admin users
• [x] Edit access limitation for Admin roles across Sequencescape and Limber LIMS
• [x] Maintain an audit trail of edits viewable in the plate/tube event section
• [x] Changes made to the retention instruction metadata need to be available for samples across Tubes and Plates
• [x] Identify whether multiple user-stories are required to implement the solution

Dependencies This story is blocked by the following dependencies:

• #<issue_no.>
• sanger/#<issue_no.>

References This story has a non-blocking relationship with:

• #<issue_no.>
• sanger/#<issue_no.>

Additional context Add any other context or screenshots about the feature request here.

[SujitDey2022]

@KatyTaylor as discussed in the previous team backlog refinement session, I have created the draft research story for your review and update purpose.

[dasunpubudumal]

Achieving the use-case through sequencescape

Approach I: Create a new table for retention instructions

New tasks required

1. Prepare retention_instructions to be stored in a separate table and update the Labware page to display retention instructions in a separate template.

• [ ] Create a new table retention_instructions in sequencescape , and the corresponding model RetentionInstruction. It should have the following attributes:
  • id
  • instruction
  • asset_id
  • created_at
  • updated_at
• [ ] Update retention instructions to be stored in retention_instructions table. Retention instructions are currently stored in custom_metadata.
  • [ ] Update app/models/labware.rb to have a ~~has_many relationship~~ has_one relationship with retention_instructions table, with asset_id as the foreign key.
• [ ] Update app/sequencescape_excel/sequencescape_excel/specialised_field/retention_instruction.rb to store new retention instructions in the new table.
• [ ] Create a new partial template _retention_instructions.html.erb that has a similar structure as app/views/labware/_metadata.html.erb to display retention instructions. Include this template in app/views/labware/show.html.erb and display retention instructions in this template.

---

2. Prepare an audit trail for retention_instructions

• [ ] Create Event::RetentionInstructionEvent that extends Event parent class. This needs to have functions that persists event records (both for updates and creations).
• [ ] Update RetentionInstruction model to extend EventfulRecord module. Add the following lines of code into the RetentionInstruction model:

has_many_events do
  event_constructor(:retention_instruction_created!, Event::RetentionInstructionEvent, :created_retention_instruction!)
  event_constructor(:retention_instruction_updated!, Event::RetentionInstructionEvent, :updated_retention_instruction!)
end

created_retention_instruction and updated_retention_instruction are functions explained in the 1st point. has_many_events declares a has_many relationship with events table.

This procedure could be used to re-use the EventfulRecord module that was developed to emit events. This is the exact same procedure carried out when saving sample histories for labware. The files related to the process of labware:

1. app/controllers/samples_controller.rb
2. app/views/samples/history.html.erb

---

3. Update retention instructions template to be edited by admin users

• [ ] Display an Edit Retention Instructions button in _retention_instructions.html.erb template which is disabled for any other user role other than administrator roles. Current user role can be extracted by using current_user helper function which is globally exposed through the AuthenticatedSystem module (which is included into ApplicationController). We can use contenteditable="true" for HTML element td to make the table editable without the Edit Retention Instruction button.
• [ ] Prepare and wire in the controller methods to save updated retention instructions.

---

4. Back-fill the retention_instruction table using data in custom_metadata

Backfill the table using the asset_id in custom_metadatum_collection. This will require a join between custom_metadata and custom_metadatum_collections.

SELECT cm.key, cm.value, cmc.asset_id
FROM <sequencescape-db>.custom_metadata cm INNER JOIN <sequencescape-db>.custom_metadatum_collections cmc ON cm.custom_metadatum_collection_id = cmc.id
WHERE cm.key = 'retention_instruction';

Approach II: Use the existing table custom_metadata for retention instructions

New tasks required

1. Update create_custom_metadatum_collection function to store in the events table for audits

Store a record in events table when new retention instructions are created. This might require updating check_and_update_existing_metadatum_collection function as well (for updates).

2. Update retention instructions template to be edited by admin users

• [ ] Create a new partial template _retention_instructions.html.erb that has a similar structure as app/views/labware/_metadata.html.erb to display retention instructions. Include this template in app/views/labware/show.html.erb and display retention instructions in this template.
• [ ] Display an Edit Retention Instructions button in _retention_instructions.html.erb template which is disabled for any other user role other than administrator roles. Current user role can be extracted by using current_user helper function which is globally exposed through the AuthenticatedSystem module (which is included into ApplicationController). We can use contenteditable="true" for HTML element td to make the table editable without the Edit Retention Instruction button.

• [ ] Prepare and wire in the controller methods to save updated retention instructions.

[KatyTaylor]

Hi @dasunpubudumal, thanks for the readable and detailed update.

General questions:

• Is the 'events' table linked to the Events Warehouse? Specifically, is it going to create an unintended event in the warehouse if you insert a record into that table? I can't remember... but can help you look if you're not sure.

Questions on approach 1:

• Why the has_many relationship? There should only be one 'retention instruction' for a particular labware. (To be fair, I'm not sure whether that's currently enforced or not with the current model, using custom metadata.)
• Please don't create another field called asset_id! I'm trying to get rid of the asset terminology! (See https://github.com/sanger/sequencescape/issues/4072) This would be linking to the labware table, right? So should be called labware_id.

Questions on approach 2:

• I presume you would create custom behaviour for custom metadata where the key is a specific string, rather than affecting all custom metadata records? e.g. 'store a record in events if the key = 'retention_instruction' ?
• In the UI, won't you have the field displayed both in app/views/labware/_metadata.html.erb and _retention_instructions.html.erb?

Potential approach 3 (this is what I thought you were suggesting in stand up):

• Move retention instructions out of custom metadata, into a new field against an existing table. This could be the labware table itself, or an existing table with a 1:1 relationship... not sure there is one. There's plate_metadata, but that presumably doesn't work for tubes.

What's your preferred option? I would go with 2 or 3.

[dasunpubudumal]

Hi @KatyTaylor, thanks for the quick response!

Is the 'events' table linked to the Events Warehouse? Specifically, is it going to create an unintended event in the warehouse if you insert a record into that table? I can't remember... but can help you look if you're not sure.

@yoldas warned me about the event warehouse, and I could not get into looking at it. I will see how this behaves and will let you know.

Why the has_many relationship? There should only be one 'retention instruction' for a particular labware. (To be fair, I'm not sure whether that's currently enforced or not with the current model, using custom metadata.)

You're correct. It is actually not a many relationship. I was focusing on another example (samples in this case, which exhibits a similar behaviour for auditing) and mistakenly written it as a has_many. It should be corrected to has_one. When we try to do a plate submission with different retention instructions for samples, the UI returns an error saying that only one type of instruction can be there for a submission. The validation for plates is in app/sample_manifest_excel/sample_manifest_excel/upload/processor/plate.rb.

Please don't create another field called asset_id! I'm trying to get rid of the asset terminology! (See https://github.com/sanger/sequencescape/issues/4072) This would be linking to the labware table, right? So should be called labware_id.

Affirmative. I remember the discussion you had in the standup, and to be frank, totally forgot about it. labware_id makes sense.

I presume you would create custom behaviour for custom metadata where the key is a specific string, rather than affecting all custom metadata records? e.g. 'store a record in events if the key = 'retention_instruction' ?

Yes, because there are multiple types of metadata that exist. Here's a list of unique keys that exist in our sequencescape production database.

Therefore, we would have to use the key attribute to filter out the retention instructions from custom metadata.

In the UI, won't you have the field displayed both in app/views/labware/_metadata.html.erb and _retention_instructions.html.erb?

That's interesting. Technically, if we aren't separating retention instructions from custom metadata, I suppose we should display the instructions in both app/views/labware/_metadata.html.erb and _retention_instructions.html.erb. In terms of the UX, it might a bit redundant to display retention instructions in both collapsables, but if we think about the entities themselves, it makes sense to have the retention instructions in both the places.

What's your preferred option? I would go with 2 or 3.

I think if we have a separate table for retention instructions, we could easily re-use existing auditing behaviour that is already implemented in places like app/models/sample.rb. BUT, that would mean we have to backfill data for existing records in custom_metadata to the new table, which is going to be an additional task.

Even though I think a new table would tidy up the implementation, I do not know whether it makes sense logically to have one. If it is logical, I suppose we could have one. But, that would also involve the additional task of backfilling the new table for existing records. In terms of achieving the tasks discussed in the story, I suppose the easiest (and probably the fastest) way is to have the instructions in the same metadata table, use key attribute to filter out retention_instructions (for the UI and for audits), and use the events table to record the audits.

[KatyTaylor]

OK, good to know you meant to write a has_one relationship, that makes more sense.

If we do move where retention instructions are stored, it's worth putting an acceptance criteria to search the codebase for places that retrieve it. e.g. def obtain_retention_instructions in labware.rb and def column_headers in location_report.rb.

I don't think the backfilling of data is a big deal in the grand scheme of things - won't take too long I think - I wouldn't let that put you off. Happy for you to go with whichever design feels best to you! Also happy to have further chats to discuss it if that would help.

A couple more options for the UI:

Include a retention instructions field in the Labware Edit page (conditionally based on user role). Below screenshot shows existing Labware Edit page:

Add a new link on the right hand side of the Labware show page (conditionally based on user role), called 'Set retention instructions' or similar, which takes you to a special page for it. Below screenshot shows actions you can do listed as links on the right hand side of the Sample show page (e.g. 'Flag for public release'):

[dasunpubudumal]

Hi @KatyTaylor,

If we do move where retention instructions are stored, it's worth putting an acceptance criteria to search the codebase for places that retrieve it. e.g. def obtain_retention_instructions in labware.rb and def column_headers in location_report.rb.

Yep, that was what I thought. It'd be bad if we move the records and miss refactoring where the retention instructions are stored and/or retrieved.

How is the backfilling process done here generally? My experiences of backfilling involve writing a couple of Python scripts that consume the current records and persist them in the new table. It wasn't that bad, but the DBAs (and sometimes, Python runtime also) weren't happy when the volume was large.

I think the second option - the one with "Set retention instructions" - from your screens would be better. My initial thoughts were to use the collapsable (with Retention Instructions) that displays the instructions as the view to update them as well, but I guess directing the user to a new page makes it cleaner - and probably a bit more understandable from the user's perspective.

---

Before we make the final decision, do you think it makes logical sense to separate retention instructions to a new table? I don't think this is required to implement what is intended in this story. But if it does make sense, we might as well go with a new table for retention instructions. (Well I suppose I'm trying to answer my own question "Why would you create a new table when it could be done using the schema you already have?")

[KatyTaylor]

How is the backfilling process done here generally? My experiences of backfilling involve writing a couple of Python scripts that consume the current records and persist them in the new table. It wasn't that bad, but the DBAs (and sometimes, Python runtime also) weren't happy when the volume was large.

Yep we would generally use this approach - but Ruby makes more sense here - a small script we can run from the Rails console while ssh'd onto the server. We can test it on training (which has a full snapshot of prod data), and could run it by the DBAs too before going to prod.

I think the second option - the one with "Set retention instructions" - from your screens would be better. My initial thoughts were to use the collapsable (with Retention Instructions) that displays the instructions as the view to update them as well, but I guess directing the user to a new page makes it cleaner - and probably a bit more understandable from the user's perspective.

Yes I think I agree that's probably the cleanest UX. The collapsible idea is quite neat, but none of the rest of that page body is editable (except for a file upload section, which is a bit different), so I feel it would be a bit out of place.

Before we make the final decision, do you think it makes logical sense to separate retention instructions to a new table? I don't think this is required to implement what is intended in this story. But if it does make sense, we might as well go with a new table for retention instructions. (Well I suppose I'm trying to answer my own question "Why would you create a new table when it could be done using the schema you already have?")

My thoughts on the design are:

• Using custom metadata was fine and easy when we didn't have these specific requirements around permissions and auditing. It feels messy having to create functionality specifically for custom metadata 'where the key = a specific string'.
• An extra table feels a bit like overkill when we essentially just need 1 field. But, I see your point - "we could easily re-use existing auditing behaviour that is already implemented in places like app/models/sample.rb" - although I haven't looked at the code here, and what the alternative is like if we don't have a separate table.
• I still think the option of adding a single new field to the Labware table seems decent - like a compromise between the two. (Although again, I'm not very familiar with what the permissions and auditing requirements would involve for this option)

It's up to you if you're comfortable making the decision. If you want my opinion, I'd probably go for adding a new field to Labware. You could also set up a quick team meeting and present the options if you like?

[dasunpubudumal]

Cool. So our current options are as follows:

1. Keep the existing structure where retention instructions are stored in custom_metadata, and use the events table for audits. Permissions are controlled through view where a link is displayed to edit retention instructions if and only if the user is an administrator.
2. Introduce a new table for retention instructions where the retention instructions are stored upon submission. This table will have to be backfilled for existing data. Current behaviour will also needs to be adjusted in all places where retention instructions are retrieved and updated. Permissions and audits follow the same scheme as the 1st point.
3. Introduce a new column retention_instruction in labware table to store retention instruction for each labware. This column will have to be backfilled for existing data. Current behaviour will also needs to be adjusted in all places where retention instructions are retrieved and updated. Permissions and audits follow the same scheme same as the 1st point.