Remove first_round methods from rules files
#1224
Conversation
| @@ -27,7 +26,7 @@ class HBat(Base): | |||
| ID = 'H_BAT' | |||
| start_diff = 20 | |||
|
|
|||
| def next_round(self, session): | |||
| def next_round(self, session: Session): | |||
There was a problem hiding this comment.
In the other rules (above here in the PR), I see you do a check for rounds passed to equal zero and then to return intro_explainer. Why not here? :-)
There was a problem hiding this comment.
We have some recurring practice views here, which is tracked by final_score. Another round of practice will still be preceded by the intro_explainer, even if we already have results for this session.
| questions = self.get_questionnaire(session) | ||
| if questions: | ||
| return questions | ||
| return [self.intro_explainer(), *questions] |
There was a problem hiding this comment.
I had to look this up because it looks kind of counter intuitive: If there are no questions in the questionnaire, return the final action / debriefing? Would it be an idea to rename this or the base method sometime in the future?
Either something like:
# backend/experiment/rules/rhythm_battery_final.py
open_questions = self.get_questionnaire(session)
if open_questions:
return [self.intro_explainer(), *open_questions]or
# backend/experiment/rules/rhythm_battery_final.py
questions = self.get_questionnaire(session, unanswered=True)
# or
questions = self.get_unanswered_questionnaire(session)Or at least update the method's docstring to make explicit that it returns open questions from a questionnaire and not all questions:
# backend/experiment/rules/base.py
def get_questionnaire(self, session, randomize=False, cutoff_index=None):
''' Get a list of open questions to be asked in succession for a given session'''| explainer = Explainer( | ||
| instruction=_( | ||
| 'General listening instructions:'), | ||
| steps=[ | ||
| Step(_( | ||
| "To make sure that you can do the experiment as well as possible, please do it a quiet room with a stable internet connection."), | ||
| ), | ||
| Step(_("Please use headphones, and turn off sound notifications from other devices and applications (e.g., e-mail, phone messages)."), | ||
| )], | ||
| step_numbers=True, | ||
| button_label=_('Ok') | ||
| ) | ||
| actions.append(self.intro_explainer()) | ||
| actions.append(explainer) |
There was a problem hiding this comment.
I guess we can also combine this explainer into the intro_explainer method, or not?
There was a problem hiding this comment.
All other methods just return the one explainer object. I'm wondering whether it would be good to add and document / type this method to base.py, but then we would always have an Explainer in place unless we override.
backend/experiment/rules/tafc.py
Outdated
| first_round() and next_round() are required methods for the class. | ||
| next_round() is a required method for this class. |
There was a problem hiding this comment.
Is this comment even necessary? :D
|
|
||
| try { | ||
| const newSession = await createSession({ block, participant, playlist }) | ||
| setSession(newSession); | ||
| return newSession; | ||
| } | ||
| catch (err) { | ||
| setError(`Could not create a session: ${err}`, err) | ||
| return; | ||
| }; |
There was a problem hiding this comment.
If you delete this, you might as well remove the (now unused) import and the createSession function from frontend/src/API.ts
| rules=Hooked.ID, | ||
| rules=RhythmBatteryIntro.ID, |
There was a problem hiding this comment.
What is the reason for this change?
There was a problem hiding this comment.
Hooked expects a playlist to be present, RhythmBatteryIntro doesn't.
| @@ -30,7 +30,7 @@ def get_practice_views( | |||
| previous_results = session.result_set.order_by('-created_at') | |||
| if not results_count: | |||
| # first practice trial | |||
| return trial_callback(session, trial_condition, difficulty) | |||
| return [intro_explainer, practice_explainer(), trial_callback(session, trial_condition, difficulty)] | |||
There was a problem hiding this comment.
I had to look this up since it confused me a bit to find a variable named intro_explainer together with a function named practice_explainer. Can we perhaps rename practice_explainer to get_practice_explainer? While we're at it, perhaps it would be a good idea to rename all intro_explainer methods in the rules files to get_intro_explainer in the future?
Working towards #973. I tested all block rules which don't inherit their
next_roundmethod from other rules classes, and fixed the issues I could find. What this branch also does is add adefault_consent_filemember to all rules classes, in prepration of #1211.