Y24-149 exhaust samples moved to destroyed location #2121

seenanair · 2024-12-16T18:00:17Z

Changes proposed in this pull request

Exhaust library volumes for all barcodes scanned to destroyed location

Instructions for Reviewers

[All PRs] - Confirm PR template filled
[Feature Branches] - Review code
[Production Merges to main]
- Check story numbers included
- Check for debug code
- Check version

…BARCODE variable

…estroyed location barcodes

…on logic

…hausting samples if destroyed location is scanned

…larity

…d-location

BenTopping

Good start, couple questions.

Do we have some way in Traction UI of representing exhausted libraries. Is an exhausted library just one thats volume equals its used_volume or does it also have to be destroyed? Might be useful to have an exhausted badge or something?

src/services/labwhere/client.js

BenTopping · 2024-12-17T14:28:25Z

src/stores/pacbioLibraries.js

-
+    async fetchLibraries(filterOptions) {
+      const { success, data, meta, errors, libraries, tubes, tags, requests } =
+        await fetchLibraries({


I think this naming should be changed so its clear this isnt a recursive function.

BenTopping · 2024-12-17T14:33:19Z

src/stores/utilities/pacbioLibraries.js

What decides whether a method goes in utilities or goes into the store? I normally consider utilities as data formatting, etc but some of these make requests to the service as well. Not sure whether that belongs better in the store or not.

I think the primary aim is to enable methods that format data, specifically those requiring additional information, to return the expected data. For example, the librariesArray method requires tubes, requests, and tags to return the information it expects. Splitting these into separate utility functions allows for easier unit testing locally, as it is much trickier to test this functionality using the factory.

I have splitted other methods based on how they are reused across different parts of the application. For instance, fetching libraries is used within the store, but it is also required in other places that do not need to rely on the store method or store state. By splitting it out, these methods can operate independently. Fetching can now be done outside the store, and the same method can be used within store to set store values as needed.

Another approach could have been to split these internally into two separate methods within the store - one method explicitly to fetch information and another method that uses this method to set the store state. However, I felt that separating them into utilities makes the logic clearer and keeps the fetchLibraries method more generic.

Hope it makes sense. Please share your thoughts.

It might be worth a general review to set a standard so we know where to put stuff.

We do have situations where we have calls to the service outside the store but maybe we should think about moving all calls to services in the services folder and standardising naming to remove any ambiguity.

BenTopping · 2024-12-17T14:40:11Z

src/views/LabwhereReception.vue

+        location_barcode.value,
+        uniqueBarcodesArray.value,
+      )
+      if (success && exhaustedLibraries.length > 0) {


This message formatting should probably be handled by exhaustLibraryVolumeIfDestroyed as exhaustedLibraries are only used for the message.

Again I have noticed this pattern where we are checking the length as well as success. It seems pertinent to move it handle response with a flag e.g. hasData.

seenanair · 2024-12-17T14:53:25Z

Good start, couple questions.

Do we have some way in Traction UI of representing exhausted libraries. Is an exhausted library just one thats volume equals its used_volume or does it also have to be destroyed? Might be useful to have an exhausted badge or something?

I thought the same.
At present, there is no representation of exhausted libraries.
It would be good

to have an indication to show whether a library is exhausted and
to disable or doesn't allow editing the volume for exhausted libraries, as it doesn’t make sense to allow it again.

stevieing · 2024-12-18T10:11:33Z

src/stores/utilities/pacbioLibraries.js

It might be worth a general review to set a standard so we know where to put stuff.

We do have situations where we have calls to the service outside the store but maybe we should think about moving all calls to services in the services folder and standardising naming to remove any ambiguity.

stevieing · 2024-12-18T10:13:30Z

src/views/LabwhereReception.vue

+        location_barcode.value,
+        uniqueBarcodesArray.value,
+      )
+      if (success && exhaustedLibraries.length > 0) {


Again I have noticed this pattern where we are checking the length as well as success. It seems pertinent to move it handle response with a flag e.g. hasData.

stevieing · 2024-12-18T10:15:20Z

tests/unit/stores/utilities/pacbioLibraries.spec.js

+} from '@/stores/utilities/pacbioLibraries.js'
+import PacbioLibraryFactory from '@tests/factories/PacbioLibraryFactory.js'
+import { beforeEach, describe, expect, it } from 'vitest'
+import { libraryPayload } from '../../../../src/stores/utilities/pacbioLibraries'


Can we add a file type to pacbioLibraries?

…ources

…bwhere client

…ies function

stevieing

I have added lots of advisory comments.

I struggled a bit to understand what was going on as there is a lot of mocking.

I don't think these comments should stop deployment from branch if it is working.

stevieing · 2025-01-07T10:10:01Z

.env.development

+VITE_ENVIRONMENT=development
+VITE_DESTROYED_LOCATION_BARCODE=lw-destroyed-217


Are youn planning to add the destroyed location to the deployment project or just the .env files?

It is already added in deployment project https://github.com/sanger/deployment/pull/533

stevieing · 2025-01-07T10:10:23Z

.env.development

@@ -2,7 +2,8 @@ VITE_TRACTION_BASE_URL=http://traction
 VITE_TRACTION_GRAPHQL_URL=http://traction/graphql
 VITE_PRINTMYBARCODE_BASE_URL=http://printmybarcode
 VITE_SAMPLEEXTRACTION_BASE_URL=http://sampleextraction
-VITE_LABWHERE_BASE_URL=http://labwhere
+VITE_LABWHERE_BASE_URL=http://localhost:3003


I think it is better to do this in .env.development.local

There is an e2e test to check for volume exhaustion when the entered location barcode matches the destroyed location barcode, as specified in the corresponding .env file. This test will fail during CI if the correct configuration is not set. Is there are any better approaches to handle this scenario?

Ah ok. That is a problem. How does it work for all of our other api calls. We don't set traction url to a real one? It sounds like the intercept is not being handled correctly maybe?

Here, we are simply comparing the entered barcode with the value of the environment variable read within the method. I think this is different from other cases where we can intercept commands to mock network requests and responses, eliminating the need for the real ones. I tried various approaches, such as using Cypress config to define a value, but it didn't work

I understand now. It is the barcode you are checking which is why it needs to be in .env

stevieing · 2025-01-07T10:20:37Z

src/services/labwhere/client.js

+  exhaustLibrayVolume,
+  formatAndTransformLibraries,
+} from '@/stores/utilities/pacbioLibraries.js'
+import { getPacbioLibraryResources } from '@/services/traction/PacbioLibrary.js'

 const labwhereFetch = FetchWrapper(import.meta.env['VITE_LABWHERE_BASE_URL'], 'LabWhere')


Advisory: Creating the variables like this rather than using dependency injection makes it more difficult to test which is why you need to mock fetch globally I think!

A different way to do it would be to pass a type into the function which could have the various bits of configuration.

stevieing · 2025-01-07T10:23:29Z

tests/unit/services/labwhere/client.spec.js

-describe('client', () => {
-  const mockFetch = vi.fn()
-  const labwhereLocationsFactory = LabwhereLocationsFactory()
+beforeEach(() => {


This is what I am referring to above.

We are essentially mocking fetch globally because of what we are doing in the client.

stevieing · 2025-01-07T10:25:20Z

tests/unit/services/labwhere/client.spec.js

+  ]
+
+  beforeEach(() => {
+    vi.clearAllMocks()


Not entirely sure what is going on but would be slightly concerned that we are pretty much mocking all of the functions. It would be good to know why. This could create technical debt.

The ones you mentioned are not included as part of this PR.
The current implementation of the LabWhere client methods and FetchWrapper directly calls the fetch API to interact with LabWhere services. I believe this approach was taken because the LabWhere APIs do not always return JSON responses, and the client methods are designed as wrappers around these APIs to handle various response formats and content types.
These methods are globally mocked since all exported methods involve calling LabWhere, and the tests need to simulate different response formats for different methods. As you mentioned, it could likely be rewritten to pass in a fetch function, allowing tests to be written without relying on global mocking.

stevieing

Nice piece of work.

I think we could also use dependency injection for the destroyed location barcode and turn it into a type but I think you have done more than enough on this for now. There will be more work on location tracking so we can look again in the future.

…d-location

seenanair added 16 commits December 16, 2024 10:03

Update environment configuration files to include DESTROYED_LOCATION_…

0ab2553

…BARCODE variable

update(client.js): Add exhaustSamplesIfDestroyed function to handle d…

ecae3df

…estroyed location barcodes

refactor(pacbioLibraries): Simplify library fetching and transformati…

5d9c54c

…on logic

update(pacbioLibraries): Add library validation and fetching functions

2f76ac3

update(LabwhereReception): Integrate exhaustSamplesIfDestroyed for ex…

6aaa5a2

…hausting samples if destroyed location is scanned

Add unit tests

0092a8a

rename: Update function name to exhaustLibraryVolumeIfDestroyed for c…

47737a0

…larity

Update pacbioLibraries.js

2f7e594

Update LabwhereReception.vue

1cd5b7c

tests

6ffb245

refactor: remove createLibrariesArray

f77f9e8

Update pacbioLibraries.spec.js

c96544e

Update visit_labwhere_reception_page.cy.js

9546b61

added documentation and prettier updates

54d03a4

Merge branch 'develop' into y24-149-exhaust-samples-moved-to-destroye…

12ea25e

…d-location

update: removed v2 namespace

c207430

seenanair requested review from BenTopping and stevieing December 17, 2024 10:38

BenTopping reviewed Dec 17, 2024

View reviewed changes

stevieing previously approved these changes Dec 18, 2024

View reviewed changes

seenanair added 5 commits January 3, 2025 14:35

feat: add PacbioLibrary service for fetching and updating library res…

f9bcd24

…ources

refactor: replace fetchLibraries with getPacbioLibraryResources in la…

c99b7cb

…bwhere client

refactor: update library resource handling in pacbioLibraries store

8a46b56

refactor: simplify library update logic and remove unused fetchLibrar…

995d5cc

…ies function

test updates

c550d0c

seenanair dismissed stevieing’s stale review via c550d0c January 3, 2025 14:36

seenanair marked this pull request as draft January 6, 2025 11:27

seenanair added 2 commits January 6, 2025 16:15

feat: add exhausted badge to volume cell in PacbioLibraryIndex component

df19bef

test updates

9bc7b99

seenanair marked this pull request as ready for review January 6, 2025 16:15

seenanair requested review from BenTopping and stevieing January 6, 2025 16:20

stevieing requested changes Jan 7, 2025

View reviewed changes

seenanair added 3 commits January 7, 2025 12:01

Update .env.development

217a3eb

Update .env.development

d55f4bd

refactor: dependency injection to avoid global fetch mocking

a7b7160

seenanair requested a review from stevieing January 7, 2025 15:25

stevieing approved these changes Jan 8, 2025

View reviewed changes

Merge branch 'develop' into y24-149-exhaust-samples-moved-to-destroye…

b468bfc

…d-location

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Y24-149 exhaust samples moved to destroyed location #2121

Y24-149 exhaust samples moved to destroyed location #2121

seenanair commented Dec 16, 2024 •

edited

Loading

BenTopping left a comment

BenTopping Dec 17, 2024

BenTopping Dec 17, 2024

seenanair Dec 17, 2024

stevieing Dec 18, 2024

BenTopping Dec 17, 2024

stevieing Dec 18, 2024

seenanair commented Dec 17, 2024

stevieing Dec 18, 2024

stevieing Dec 18, 2024

stevieing Dec 18, 2024

stevieing left a comment

stevieing Jan 7, 2025

seenanair Jan 7, 2025

stevieing Jan 7, 2025

seenanair Jan 7, 2025

stevieing Jan 7, 2025 •

edited

Loading

seenanair Jan 7, 2025

stevieing Jan 7, 2025

stevieing Jan 7, 2025

stevieing Jan 7, 2025

stevieing Jan 7, 2025

seenanair Jan 7, 2025

stevieing left a comment

		VITE_ENVIRONMENT=development
		VITE_DESTROYED_LOCATION_BARCODE=lw-destroyed-217

Y24-149 exhaust samples moved to destroyed location #2121

Are you sure you want to change the base?

Y24-149 exhaust samples moved to destroyed location #2121

Conversation

seenanair commented Dec 16, 2024 • edited Loading

Changes proposed in this pull request

Instructions for Reviewers

BenTopping left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seenanair commented Dec 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevieing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevieing Jan 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevieing left a comment

Choose a reason for hiding this comment

seenanair commented Dec 16, 2024 •

edited

Loading

stevieing Jan 7, 2025 •

edited

Loading