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

Unending spinning of dataset images #807

Merged

Conversation

ddjnw1yu
Copy link
Collaborator

Description

All the changes are focused on fixing the unending spinning issue mentioned in ticket Investigate unending spinning of dataset images for all datasets

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

I have done manual testing. Accessed all the dataset galleries and checked whether all spinning icons were gone.

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have utilized components from the Design System Library where applicable
  • I have added or updated unit tests that prove my fix is effective or that my feature works
  • I updated any relevant QC tests to match my changes found here: https://docs.google.com/document/d/1tlV89PMOv8XlmC7LVqifi7NfQ5-AYN8DuEQpmO7O_2Q

Failed request and incorrect name structure will cause forever loading
If no file suffix, the name will be undefined
The thumbnail response can be empty string. When this happens, the default thumbnail will not be used.
When duplicate images exist, except the first image request, all others will have no responses.
@ddjnw1yu ddjnw1yu requested a review from egauzens April 11, 2024 23:05
Copy link

cypress bot commented Apr 11, 2024

1 failed test on run #60 ↗︎

1 46 4 0 Flakiness 0

Details:

Remove duplicate images from biolucida dataset
Project: SPARC Portal E2E testing Commit: a5aabc9a64
Status: Failed Duration: 09:14 💡
Started: Apr 11, 2024 11:04 PM Ended: Apr 11, 2024 11:13 PM
Failed  test/cypress/e2e/homepage.cy.js • 1 failed test

View Output Video

Test Artifacts
Homepage > Footer Screenshots Video

Review all test suite changes for PR #807 ↗︎

Copy link
Contributor

@egauzens egauzens left a comment

Choose a reason for hiding this comment

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

I do not think we want to remove the check for an image title. When applying these changes, clicking on the first image in the gallery of dataset 205 takes me to: /datasets/file/205/1?path=files/derivative/sub-7/sub-7-sam-1/Dog7%2020X%20Caspase-3%20(1).jp2 but does not show an associated biolucida viewer. It looks like the name of the image should be: Dog 20X Caspase-3 (1).jp2

@@ -104,7 +104,7 @@ export default {
},
computed: {
isReady() {
return this.data.title && (this.thumbnail || this.useDefaultImg) && (this.data.link || this.data.userData)
Copy link
Contributor

Choose a reason for hiding this comment

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

why are we removing the check for the title? Images should have a title associated with them

Copy link
Collaborator Author

@ddjnw1yu ddjnw1yu Apr 15, 2024

Choose a reason for hiding this comment

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

Due to the failed biolucida requests, the file name will be undefined. The main reason for removing the title check is to avoid unending spinning caused by this. This may cause some inconvenience for people who are doing the portal QC. I updated the existing biolucida testing which will run through all the datasets and help to generate a report of related issues. You will be able to find an example report in this ticket.

@@ -808,6 +811,9 @@ export default {
const name = response.name
if (name) {
this.$set(item, 'title', name.substring(0, name.lastIndexOf('.')))
if (name.lastIndexOf('.') === -1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this is happening because we are removing the check for the title. I believe every image should have a title. If an image does not have one, we need to figure out why biolucida is not returning one

Copy link
Collaborator Author

@ddjnw1yu ddjnw1yu Apr 15, 2024

Choose a reason for hiding this comment

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

The change here is because of the file name structure. The original code's default file name format should end with a suffix, e.g. .jp2 in Dog 20X Caspase-3 (1).jp2. However, in some datasets, the file name will not have the suffix (some other reason caused this). name.substring(0, name.lastIndexOf('.')) will be an empty string. For example, some image files in dataset 178 (the issue is caught in the above example report).

@ddjnw1yu
Copy link
Collaborator Author

ddjnw1yu commented Apr 15, 2024

I do not think we want to remove the check for an image title. When applying these changes, clicking on the first image in the gallery of dataset 205 takes me to: /datasets/file/205/1?path=files/derivative/sub-7/sub-7-sam-1/Dog7%2020X%20Caspase-3%20(1).jp2 but does not show an associated biolucida viewer. It looks like the name of the image should be: Dog 20X Caspase-3 (1).jp2

I understand that removing the check for an image title is not an ideal solution. The main reason why the name does not show in the gallery card and biolucida viewer doesn't show the image for dataset 205 here is because permission denied or image id does not exist on biolucida.

@egauzens
Copy link
Contributor

egauzens commented Apr 16, 2024

I do not think we want to remove the check for an image title. When applying these changes, clicking on the first image in the gallery of dataset 205 takes me to: /datasets/file/205/1?path=files/derivative/sub-7/sub-7-sam-1/Dog7%2020X%20Caspase-3%20(1).jp2 but does not show an associated biolucida viewer. It looks like the name of the image should be: Dog 20X Caspase-3 (1).jp2

I understand that removing the check for an image title is not an ideal solution. The main reason why the name does not show in the gallery card and biolucida viewer doesn't show the image for dataset 205 here is because permission denied or image id does not exist on biolucida.

Do we know why we are getting those errors from Biolucida? My main concern is that the image being in the gallery implies that it has a viewer associated with it, but when they click on it to view it, it will not show up

@egauzens egauzens requested a review from alan-wu April 16, 2024 15:47
@jgrethe
Copy link
Collaborator

jgrethe commented Apr 16, 2024

@egauzens I remember Maci mentioning something about some older datasets using a "local" path for some of the images instead of the correct dataset path (if I remember correctly).

@egauzens egauzens self-requested a review April 16, 2024 20:10
Copy link
Contributor

@egauzens egauzens left a comment

Choose a reason for hiding this comment

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

It was decided at tech leads to merge this in for the time being while Biolucida looks into the error issues since it's better to have something that doesn't display quite right as opposed to entirely broken

@egauzens egauzens merged commit 4b1f122 into nih-sparc:master Apr 16, 2024
0 of 2 checks passed
@ddjnw1yu ddjnw1yu deleted the unending-spinning-of-dataset-images branch April 23, 2024 03:58
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.

3 participants