-
Notifications
You must be signed in to change notification settings - Fork 1
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
Add a data permission question to the new-project wizard #462
Conversation
|
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. |
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 ( |
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.
As discussed on slack, this works just as expected - nicely done, thanks @joanise !
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
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.
b429299
to
04b2c38
Compare
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_1
instead oflabel: dataset_0
inconfig/everyvoice-shared-data.yaml
. Is that OK?Priority?
alpha
Tests added?
yes
How to test?
Run
everyvoice new-project
and alternately answer no/yes to the permission question, see what happens during the wizard and in the output configuration.Confidence?
high