Handle non-valid dataset paths #1129
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jameshadfield
requested changes
May 25, 2020
There was a problem hiding this comment.
Thanks for this! Moving the server to use redirects is great, however the changes to the client present some problems for nextstrain.org.
- This PR reorders the options in each select menu such that the selected entry is on the top. I can see why this is nice, especially for vanilla auspice where the options are alphabetically ordered (arriving via the
getAvailiableAPI call). However the nextstrain.org server returns these options in a particular order, and thus moving the selected entry to the top is not desirable, 👇
- This PR modifies the client behavior such that if an "incomplete" dataset is asked for (e.g. you're viewing "zika" and you use the drop-downs to select "flu") then the partial path is filled out on the client side and a resulting API call is made for the "flu/avian/h5n1/ha" dataset. Instead, we want to send the incomplete path to the server and let the server redirect as necessary (functionality which this PR also adds). This allows custom servers (e.g. the nextstrain.org server) to complete partial dataset paths in a bespoke way -- for instance the above action on nextstrain.org should go to the server as "/flu" which will result in a redirect to "flu/seasonal/h3n2/ha" (as opposed to a request to the server for "flu/avian/h5n1/ha" as in this PR).
cli/server/getDatasetHelpers.js
Outdated
| /* TODO currently there must be an _exact_ match in the available datasets */ | ||
| if (!availableDatasets.map((d) => d.request).includes(requestStrToMatch)) { | ||
| throw new Error(`${requestStrToMatch} not in available datasets`); | ||
| const extendDataPathsToMatchAvailable = (res, info, availableDatasets) => { |
There was a problem hiding this comment.
This is great! Could you please rename the function to better reflect the functionality -- along the lines of fc824fc (can just cherry-pick that if you're happy with it)
There was a problem hiding this comment.
The cherry-pick merged without any conflict so I kept it 👍
There was a problem hiding this comment.
For the rest, the new commit should take care of the changes requested.
I wrote in a slightly different, more compact form but if you prefer I can rewrite it with a for cycle, it may be better for readability.
jameshadfield
approved these changes
May 28, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of proposed changes
Currently the dataset selector leads to invalid paths when used for example to switch to dengue, (because /dengue does not exist, /dengue/denv1 does, and /dengue/denv2 etc).
On the current master build of auspice this is not handled, on nextstrain.org this is handled with a res.redirect on the getDataset endpoint.
Additionally the selector switches dataset even when same dataset is reselected (useless loading).
With this PR, the selector automatically loads fully valid paths, paths identical to the current path are not reloaded, and incomplete paths are handled by auspice's server directly.
Testing
In dev mode, test for example
http://localhost:4000/dengue
http://localhost:4000/zika/nothanksjeff
http://localhost:4000/VirusThatDontExist
Also try fiddling with Dataset Selector
Thank you for contributing to Nextstrain!