-
Notifications
You must be signed in to change notification settings - Fork 3
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
autoconversion: Fix/conversion filename bug #486
Conversation
…s-text Update "no trajectories" text
… into 346-implement-production-autoconversion-ui
Co-authored-by: Megan Riel-Mehan <meganrm@gmail.com>
… into feature/add-conversion-form-overlay
…com/simularium/simularium-website into feature/add-conversion-form-overlay
… into feature/add-import-drop-down
…ularium/simularium-website into feature/add-conversion-form-overlay
… into 346-implement-production-autoconversion-ui
Feature/add import drop down
…ularium/simularium-website into feature/add-conversion-form-overlay
…github.com/simularium/simularium-website into feature/add-conversion-form-overlay
Co-authored-by: Megan Riel-Mehan <meganrm@gmail.com>
Co-authored-by: Megan Riel-Mehan <meganrm@gmail.com>
Co-authored-by: Megan Riel-Mehan <meganrm@gmail.com>
… into fix/conversion-filename-bug
src/constants/index.ts
Outdated
export const DATA_BUCKET_URL = | ||
"https://aics-simularium-data.s3.us-east-2.amazonaws.com"; | ||
"https://staging-simularium-ecs.allencell.org:443/download"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we're handling download requests through octopus now, we should just use the BACKEND_SERVER_IP
, like we do for defining the net connection settings, and remove this constant. Hard coding it to look at staging like we have here would break in production, because there won't be the same files in each for the case of autoconversion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, fixed this! We talked abou this, but I'm posting here for context for other reviewers that the .simularium
file extension needed to be appended to the fileId
earlier in the process to ensure that the download links for converted and other networked files also looked the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I may not be a redux expert, but this looks good to me!
src/constants/index.ts
Outdated
@@ -15,8 +15,7 @@ export const URL_PARAM_BASE_TYPES = "base_types.json"; | |||
export const URL_PARAM_CUSTOM_TYPES = "custom-types"; | |||
// URLs | |||
export const BASE_API_URL = `/api/${API_VERSION}`; | |||
export const DATA_BUCKET_URL = | |||
"https://aics-simularium-data.s3.us-east-2.amazonaws.com"; | |||
export const DATA_BUCKET_URL = `https://${process.env.BACKEND_SERVER_IP}:443/download`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this isn't really a bucket anymore, DOWNLOAD_URL
may be more accurate
… into fix/conversion-filename-bug
Time estimate or Size
Medium
This should be working with current viewer patch.
Problem
Converted trajectories were not downloadable, had no titles in the header, and were buggy when loaded after other trajectories.
Solution
The biggest part of this change is a new flow to receive converted files, first
handleIncomingConvertedFile
in the viewer panel and thenreceiveConvertedFileLogic
in redux.handleIncomingConvertedFile
puts the name and title into state for use in the header, download menu, etc.The
receiveConvertedFileLogic
callscontroller.changeFile
with the right arguments to keep the websocket connection we established during conversion, switching connections was causing bugs.Some of this was fixed by recent viewer PR, and this change set brings the website into alignment with viewer in how it calls controller methods like
convertTrajectory
andchangeFile
.Miscellaneous changes:
getIsNetworkedFile
selector was checking if files are inTRAJECTORIES
constant, but converted files won't always be there in auto-conversion era, removed checkgetHref
To review: