Add a data permission question to the new-project wizard #462
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
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #462 +/- ##
==========================================
+ Coverage 74.28% 74.60% +0.31%
==========================================
Files 44 44
Lines 2781 2815 +34
Branches 430 434 +4
==========================================
+ Hits 2066 2100 +34
+ Misses 630 629 -1
- Partials 85 86 +1 ☔ View full report in Codecov by Sentry. |
joanise
force-pushed
the
dev.ej/data-attestation
branch
from
7e1ab2c to
435f773
Compare
|
wow - this was super quick too! I'll have time to review this tomorrow afternoon hopefully after my evaluations! |
|
If you test this PR interactively, I suggest reviewing #463 at the same time and testing on that branch ( |
|
related feature request: #465 |
refactor: flatten the wizard tree structure and simplify multi adds - add a dummy root node to the tour tree - init the wizard with a flat list instead of a deep one branch tree - create add_steps() to simplify adding several steps at once, and accept them in order instead of reversed refactor: make the main tour a function, not a constant This matters in unit testing where we restart multiple times, sometimes with destructive effects, but it's cleaner design anyway.
…teps Also: skip checking email deliverability during unit testing
Fixes #422 Co-authored-by: Aidan Pine <[email protected]>
Also fix the other tests for the modified wizard tour structure
joanise
force-pushed
the
dev.ej/data-attestation
branch
2 times, most recently
from
752b002 to
b429299
Compare
- Move the wavs question after fully processing the filelist, including permission. - Catch the case where we don't have permission to write the directory with a friendly error message. - Make the ValidateWavsStep question use the same type of prompt as the other similar questions, for better UX consistency - If we accept a wavs dir / filelist combo with missing wav files, give a bright error message to make it clear future problems will happen. - Don't save the config if no dataset was created. - Delete the output directory in the OutputPathStep so we don't leave an empty directory lying around when we abort without saving, for any reason. (SL request, for something that's been bugging me for quite a while!) - Clarify various message texts based on SL PR review feedback.
joanise
force-pushed
the
dev.ej/data-attestation
branch
from
b429299 to
04b2c38
Compare
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.
PR Goal?
Add a data attestation step to the wizard.
Behaviour implemented: when you say "no, I don't have permission", the dataset you just started creating (with the wavs dir and the filelist) is erase from the wizard, and you jump to the end, to the "do you have more data" question where you can try again by saying yes.
The final output will include only those datasets for which you said yes to the permission question.
Fixes?
Fixes #422
Feedback sought?
General code validation. In particular, the structure of the tour is significantly modified by this PR, so a second set of eyes on that would be good.
When testing the wizard, is the behaviour as you would expect?
If you run the wizard and first say no to the permission question, then say yes to more data and then yes to the permission question the second time around, you end up with
label: dataset_1instead oflabel: dataset_0inconfig/everyvoice-shared-data.yaml. Is that OK?Priority?
alpha
Tests added?
yes
How to test?
Run
everyvoice new-projectand alternately answer no/yes to the permission question, see what happens during the wizard and in the output configuration.Confidence?
high