Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate file size/count queries to use ICAT v5 properties for investigation/dataset entities #1499

Merged
merged 52 commits into from
Feb 6, 2024

Conversation

jounaidr
Copy link
Contributor

@jounaidr jounaidr commented Jan 17, 2023

Description

With ICAT V5.0.0 the underlying file sizes and counts for dataset and investigation entities are included as properties. Previously when a file size/count was required for any table/card views, it was calculated on the fly using a query. This PR removes said query methods and where they are used and replaces them with the values directly from the received entitles.

Note that I was not able to test the changes made to dls table and card views, so the changes made to the respective files are assumed to work but should be double checked before this PR is approved. Also, the fetchSize() size buttons that were previously implemented within dls/datasetDetailsPanel.component.tsx and dls/visitDetailsPanel.component.tsx have been removed since the size is no longer calculated using the query methods mentioned. Related e2e tests have been removed as well. Maybe the file count columns should be replaced with the file sizes in dls views?

The formatCountOrSize() method within cellContentRenderers.tsx is now somewhat redundant as the formatBytes() method is now imported directly when used, so this method could possibly be refactored to only format the count (i.e formatCount()). However the method is used elsewhere so I think this is out of scope for this issue.

To Finish PR

As mentioned in review comments, some of datagateway-download downloadCartTable can also be refactored to only use one query per item in the cart. An attempt has been made, with new API to request the entire entities, and to use the new fileSize/fileCount properties within downloadCartTable. Using multiple types for Datafile Datasets and Investigations seems to be working, however as expected the file count method needs changes to identify if the entity is a datafile as count property doesn't exist. see here

Testing instructions

Check each page where a file size or count has been refactors still correctly displays the respective info.

Agile board tracking

Closes #1457

Signed-off-by: jounaidr <jounaidruhomaun@googlemail.com>
Signed-off-by: jounaidr <jounaidruhomaun@googlemail.com>
- add fileSize and fileCount props to Investigation interface
- migrate investigation sizes in isisInvestigationsCardView.component.tsx

Signed-off-by: jounaidr <jounaidruhomaun@googlemail.com>
- add fileSize and fileCount props to Dataset Datafile interfaces, set all to possibly undefined
- add fileCount prop to test Datafile
- make download button entity size nullish in isisInvestigationsCardView.component.tsx

Signed-off-by: jounaidr <jounaidruhomaun@googlemail.com>
Signed-off-by: jounaidr <jounaidruhomaun@googlemail.com>
Signed-off-by: jounaidr <jounaidruhomaun@googlemail.com>
Signed-off-by: jounaidr <jounaidruhomaun@googlemail.com>
- migrate file size in investigationSearchTable.component.tsx
- add TODO comment in cellContentRenderers.tsx formatCountOrSize() function

Signed-off-by: jounaidr <jounaidruhomaun@googlemail.com>
Signed-off-by: jounaidr <jounaidruhomaun@googlemail.com>
Signed-off-by: jounaidr <jounaidruhomaun@googlemail.com>
…size as dls hasn't upgraded to ICAT v5 yet

Signed-off-by: jounaidr <jounaidruhomaun@googlemail.com>
Signed-off-by: jounaidr <jounaidruhomaun@googlemail.com>
Signed-off-by: jounaidr <jounaidruhomaun@googlemail.com>
Signed-off-by: jounaidr <jounaidruhomaun@googlemail.com>
…ponent.tsx

Signed-off-by: jounaidr <jounaidruhomaun@googlemail.com>
…ent.tsx

Signed-off-by: jounaidr <jounaidruhomaun@googlemail.com>
Signed-off-by: jounaidr <jounaidruhomaun@googlemail.com>
Signed-off-by: jounaidr <jounaidruhomaun@googlemail.com>
- migrate dataset size in dls datasetDetailsPanel.component.tsx
- remove useDatasetSize query from datasets.tsx

Signed-off-by: jounaidr <jounaidruhomaun@googlemail.com>
…note: untested)

Signed-off-by: jounaidr <jounaidruhomaun@googlemail.com>
…note: untested)

Signed-off-by: jounaidr <jounaidruhomaun@googlemail.com>
Signed-off-by: jounaidr <jounaidruhomaun@googlemail.com>
Signed-off-by: jounaidr <jounaidruhomaun@googlemail.com>
Signed-off-by: jounaidr <jounaidruhomaun@googlemail.com>
Signed-off-by: jounaidr <jounaidruhomaun@googlemail.com>
Signed-off-by: jounaidr <jounaidruhomaun@googlemail.com>
- fix common package unit tests
- remove fetch() size button from dls pages

Signed-off-by: jounaidr <jounaidruhomaun@googlemail.com>
@@ -205,23 +195,7 @@ const VisitDetailsPanel = (
{t('investigations.details.size')}
</Typography>
<Typography>
<b>
Copy link
Contributor Author

@jounaidr jounaidr Jan 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fetch() size button removed as I believe its redundant now, however I haven't test dls pages

Signed-off-by: jounaidr <jounaidruhomaun@googlemail.com>
Signed-off-by: jounaidr <jounaidruhomaun@googlemail.com>
Signed-off-by: jounaidr <jounaidruhomaun@googlemail.com>
Signed-off-by: jounaidr <jounaidruhomaun@googlemail.com>
Signed-off-by: jounaidr <jounaidruhomaun@googlemail.com>
Signed-off-by: jounaidr <jounaidruhomaun@googlemail.com>
Signed-off-by: jounaidr <jounaidruhomaun@googlemail.com>
# Conflicts:
#	packages/datagateway-dataview/src/views/card/dls/__snapshots__/dlsDatasetsCardView.component.test.tsx.snap
#	packages/datagateway-dataview/src/views/card/dls/__snapshots__/dlsVisitsCardView.component.test.tsx.snap
#	packages/datagateway-dataview/src/views/card/isis/isisInvestigationsCardView.component.test.tsx
#	packages/datagateway-dataview/src/views/landing/isis/isisDatasetLanding.component.test.tsx
#	packages/datagateway-dataview/src/views/landing/isis/isisInvestigationLanding.component.test.tsx
#	packages/datagateway-dataview/src/views/table/__snapshots__/datasetTable.component.test.tsx.snap
#	packages/datagateway-dataview/src/views/table/datasetTable.component.test.tsx
#	packages/datagateway-dataview/src/views/table/dls/__snapshots__/dlsDatasetsTable.component.test.tsx.snap
#	packages/datagateway-dataview/src/views/table/dls/__snapshots__/dlsMyDataTable.component.test.tsx.snap
#	packages/datagateway-dataview/src/views/table/dls/__snapshots__/dlsVisitsTable.component.test.tsx.snap
#	packages/datagateway-dataview/src/views/table/dls/dlsDatasetsTable.component.test.tsx
#	packages/datagateway-dataview/src/views/table/investigationTable.component.test.tsx
#	packages/datagateway-dataview/src/views/table/isis/isisDatasetsTable.component.test.tsx
#	packages/datagateway-dataview/src/views/table/isis/isisInvestigationsTable.component.test.tsx
#	packages/datagateway-dataview/src/views/table/isis/isisMyDataTable.component.test.tsx
#	packages/datagateway-search/src/card/__snapshots__/datasetSearchCardView.component.test.tsx.snap
#	packages/datagateway-search/src/card/__snapshots__/investigationSearchCardView.component.test.tsx.snap
#	packages/datagateway-search/src/card/datasetSearchCardView.component.test.tsx
#	packages/datagateway-search/src/card/investigationSearchCardView.component.test.tsx
#	packages/datagateway-search/src/table/__snapshots__/datasetSearchTable.component.test.tsx.snap
#	packages/datagateway-search/src/table/__snapshots__/investigationSearchTable.component.test.tsx.snap
#	packages/datagateway-search/src/table/datasetSearchTable.component.test.tsx
#	packages/datagateway-search/src/table/investigationSearchTable.component.test.tsx
Signed-off-by: jounaidr <jounaidruhomaun@googlemail.com>
Signed-off-by: jounaidr <jounaidruhomaun@googlemail.com>
Copy link
Member

@louise-davies louise-davies left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's also some refactoring needed in datagateway-download - as downloadCartTable displays the size + file count of each entity in the cart to both display and accumulate the total size + file count. There's some efficiency to be had in only going through and fetching each entity in the cart once, and using the resulting Datafiles/Datasets/Investigations to derive both the sizes and counts - whereas currently we're sending two queries for each entity. lmk if you want to chat with me to explain this better.

Otherwise, just some minor comments and some things pending on knowing what DLS want


return sizes;
};

export const useInvestigationsDatasetCount = (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can also remove this function correct? As Investigation already has a fileCount field. Though I suppose that's working out the number of datafiles instead of the number of datasets. I guess we need to ask Diamond what info they'd like to display in the tables - I think originally count was only used because calculating sizes would be too inefficient...

@@ -32,6 +32,8 @@ export function formatBytes(bytes: number | undefined): string {
* @param formatAsBytes Whether to format the data as bytes, default is false
* @returns a string with either Calculating, Unknown or the formatted data
*/
//TODO: Since filesize is now given as an entity property, this function can be
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO left in here - should probably just get resolved. However, if we decide that we no longer need to fetch investigation dataset counts then this function could be removed altogether I think?

packages/datagateway-common/src/app.types.tsx Outdated Show resolved Hide resolved
jounaidr and others added 9 commits March 23, 2023 13:14
Signed-off-by: jounaidr <jounaidruhomaun@googlemail.com>
Signed-off-by: jounaidr <jounaidruhomaun@googlemail.com>
…d count

Signed-off-by: jounaidr <jounaidruhomaun@googlemail.com>
Signed-off-by: jounaidr <jounaidruhomaun@googlemail.com>
Signed-off-by: jounaidr <jounaidruhomaun@googlemail.com>
@louise-davies
Copy link
Member

I've addressed the review comments I had previously made and ensured all the tests are passing - we're now just waiting on a decision from Diamond on what they want to show in their views

@louise-davies louise-davies mentioned this pull request Jun 29, 2023
3 tasks
@louise-davies
Copy link
Member

Dataset count replaced with investigation filesize

Dataset datafile count replaced with the ICAT 5 equivalent, and add a size column

Waiting confimation from Kirsty and Greg

Copy link
Contributor

@kaperoo kaperoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me 👍

@louise-davies louise-davies merged commit f227864 into develop Feb 6, 2024
10 checks passed
@louise-davies louise-davies deleted the feature/use-icat5-file-count-#1457 branch February 6, 2024 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Switch to using ICAT 5 filesize/counts
3 participants