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

Engaging Crowds: switch to Datasette for subject browse & search #2132

Merged
merged 6 commits into from
May 12, 2021

Conversation

eatyourgreens
Copy link
Contributor

@eatyourgreens eatyourgreens commented Apr 28, 2021

Switch the subject indexing tool over to Datasette.

This PR lets you select a subject set and subject to classify. #2148 is needed in order to load the indexed subjects into the classifier.

Test URL:
http://local.zooniverse.org:3000/projects/bogden/scarlets-and-blues?env=production&demo=true
The Minutes workflow for Scarlets & Blues has subject sets which are indexed in Datasette.

Package:
app-project

Review Checklist

General

  • Are the tests passing locally and on Travis?
  • Is the documentation up to date?

Components

Apps

  • Does it work in all major browsers: Firefox, Chrome, Edge, Safari?
  • Does it work on mobile?
  • Can you yarn panic && yarn bootstrap or docker-compose up --build and app works as expected?

Publishing

  • Is the changelog updated?
  • Are the dependencies updated for apps and libraries that are using the newly published library?

Post-merging

@eatyourgreens eatyourgreens force-pushed the subject-set-search-api branch 6 times, most recently from 1b1cf27 to bea0f18 Compare May 5, 2021 14:31
@eatyourgreens eatyourgreens marked this pull request as ready for review May 5, 2021 15:04
@eatyourgreens eatyourgreens requested review from shaunanoordin and a team May 5, 2021 15:04
@eatyourgreens eatyourgreens added the enhancement New feature or request label May 5, 2021
@eatyourgreens eatyourgreens force-pushed the subject-set-search-api branch from bea0f18 to b84c247 Compare May 10, 2021 14:09
@shaunanoordin
Copy link
Member

Awright, code read looks good. I only have two comments (see above), so I'll start with the functionality tests now. Or after dinner. We'll see!

Copy link
Member

@shaunanoordin shaunanoordin left a comment

Choose a reason for hiding this comment

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

PR Review

This PR connects the "Subject Set selection" logic to an external Datasette service. This external service essentially acts as a query-able index for information on subject sets, e.g. "yo pls list all Subjects in this set in its published page order"

  • New external dependency: https://subject-set-search-api.zooniverse.org/subjects (Datasette service which lists subjects/subject sets)
  • Hardcoded values (e.g. "WF 16106 has been indexed") have been removed so logic is more correct. (i.e. pls check with the external Datasette service to see if the subject set has been indexed there)
  • ⚠️ Important note: code assumes production environment only, simply because there's no 'staging alternative' to subject-set-search-api.zooniverse.org
  • Note: the ping to the external service is done on Next.js, not client-side.

Testing

Tested on localhost, using Chrome + macOS.

URLs:

Testing:

  • Go to the /classify page
  • Choose the workflow, choose any Subject Set.
  • For the main project, this should load a list of all Subjects for the Subject Set. ("Select Subject" functionality)
    • NOTE: actually clicking on the specific Subject won't actually load it. This is a separate PR.
  • For the baseline, no differences in the functionality should be noted.

Testing looks good 👍 After adding some debug commands into the code, I can confirm that the external Datasette service is being pinged correctly by Next.js.

Advanced testing:

  • I tried to figure out what'd happen if hasIndexedSubjects() couldn't reach the external service (e.g. the service is 500-ing).
  • In theory, this should bork the main project (no indexing service = this feature shouldn't be able to work, so a functionality crash should be expected) but have no effect on the baseline project (the hasIndexedSubjects just times out and all it costs is a bit of waiting time).
  • Unfortunately, I haven't been able to devise a way to test this properly. (Substituting the URL with a fake one didn't quite pan out the way I expected.)

Main goal of this advanced test is to ensure a failure in the external dependency should not negatively affect projects that don't depend on it.

Probably nothing to worry about right now, but worth noting in case it crops up in the future.

Status

LGTM, giving this a 👍

@github-actions github-actions bot added the approved This PR is approved for merging label May 12, 2021
@eatyourgreens eatyourgreens force-pushed the subject-set-search-api branch from b84c247 to ff8e1b0 Compare May 12, 2021 08:39
@eatyourgreens
Copy link
Contributor Author

If the promises in Promise.allSettled(subject_sets.map(subjectSet => hasIndexedSubjects(subjectSet))) all reject, then I think subjectSet.isIndexed will be false by default for each set. At least, that's my reading of the tests, which set isIndexed to false by default. So I think the default behaviour, if the indexing service is down, would be to turn off subject selection in the browser. After selecting a subject set, you'd be taken straight to that set in the classifier.

On the other hand, if Promise.allSettled errors, then the error would be caught and logged, and an empty array of subject sets would be returned. That would break selection for a grouped workflow, because you must choose a set before you can classify in that case.

Encode URL query params and add a test case with reserved characters.
@eatyourgreens eatyourgreens merged commit ddef799 into master May 12, 2021
@eatyourgreens eatyourgreens deleted the subject-set-search-api branch May 12, 2021 09:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved This PR is approved for merging enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants