-
Notifications
You must be signed in to change notification settings - Fork 22
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
feat: teacher onboarding #83
Conversation
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.
Reviewed 13 of 13 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @SKairinos)
package.json
line 29 at r1 (raw file):
"dependencies": { "@react-pdf/renderer": "^4.0.0", "codeforlife": "github:ocadotechnology/codeforlife-package-javascript#portal-frontend-44",
Update to new version once released
src/pages/teacherOnboarding/StudentCredentialsTable.tsx
line 16 at r1 (raw file):
const StudentCredentialsTable: FC<StudentCredentialsTableProps> = props => { return ( <>
Page is missing the text elements:
Also we normally have the notification as well with the download button. It's common to both flows in which the CreateStudentsForm can be used so can probably be added to the reusable form as a message
src/pages/teacherOnboarding/CreateClassForm.tsx
line 21 at r1 (raw file):
} const CreateClassForm: FC<CreateClassFormProps> = ({ submitOptions }) => (
Already exists here, let's make it a reusable form like you did with the create students form
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.
Reviewable status: 11 of 13 files reviewed, 3 unresolved discussions (waiting on @faucomte97)
package.json
line 29 at r1 (raw file):
Previously, faucomte97 (Florian Aucomte) wrote…
Update to new version once released
Done.
src/pages/teacherOnboarding/CreateClassForm.tsx
line 21 at r1 (raw file):
Previously, faucomte97 (Florian Aucomte) wrote…
Already exists here, let's make it a reusable form like you did with the create students form
Done.
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.
Reviewed 2 of 2 files at r2, 4 of 4 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @SKairinos)
src/pages/teacherOnboarding/CreateSchoolForm.tsx
line 30 at r3 (raw file):
{ name: "", country: undefined,
Realised country
is not required. Needs fix on the backend I'm guessing
src/components/form/CreateClassForm.tsx
line 50 at r3 (raw file):
<Stack direction={{ sm: "row" }} gap={2}> <ClassNameField required /> {authUser.teacher.is_admin && <TeacherAutocompleteField required />}
We don't show this field in the onboarding normally. But to be honest, maybe this is part of a slightly bigger issue where this field shouldn't be shown if there's only 1 teacher in the school anyway 🤔 Could be part of a later task.
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.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @SKairinos)
src/components/form/CreateClassForm.tsx
line 50 at r3 (raw file):
Previously, faucomte97 (Florian Aucomte) wrote…
We don't show this field in the onboarding normally. But to be honest, maybe this is part of a slightly bigger issue where this field shouldn't be shown if there's only 1 teacher in the school anyway 🤔 Could be part of a later task.
Add migration to make all non-school teachers not admins, double check it has no knock-on effects
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.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @faucomte97)
src/pages/teacherOnboarding/CreateSchoolForm.tsx
line 30 at r3 (raw file):
Previously, faucomte97 (Florian Aucomte) wrote…
Realised
country
is not required. Needs fix on the backend I'm guessing
Done.
src/components/form/CreateClassForm.tsx
line 50 at r3 (raw file):
Previously, faucomte97 (Florian Aucomte) wrote…
Add migration to make all non-school teachers not admins, double check it has no knock-on effects
Done.
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.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @SKairinos)
src/pages/teacherOnboarding/CreateSchoolForm.tsx
line 30 at r3 (raw file):
Previously, SKairinos (Stefan Kairinos) wrote…
Done.
I don't see any changes?
src/components/form/CreateClassForm.tsx
line 50 at r3 (raw file):
Previously, SKairinos (Stefan Kairinos) wrote…
Done.
It had knock-on effects, make sure to run tests after creating new migrations
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.
Reviewable status: 13 of 19 files reviewed, 3 unresolved discussions (waiting on @faucomte97)
src/pages/teacherOnboarding/CreateSchoolForm.tsx
line 30 at r3 (raw file):
Previously, faucomte97 (Florian Aucomte) wrote…
I don't see any changes?
Changes are in the BE.
src/pages/teacherOnboarding/StudentCredentialsTable.tsx
line 16 at r1 (raw file):
Previously, faucomte97 (Florian Aucomte) wrote…
Page is missing the text elements:
Also we normally have the notification as well with the download button. It's common to both flows in which the CreateStudentsForm can be used so can probably be added to the reusable form as a message
Done.
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.
Reviewable status: 13 of 19 files reviewed, 3 unresolved discussions (waiting on @faucomte97)
src/components/form/CreateClassForm.tsx
line 50 at r3 (raw file):
Previously, faucomte97 (Florian Aucomte) wrote…
It had knock-on effects, make sure to run tests after creating new migrations
I don't understand what this means. Please share the link to the failed test
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.
Reviewed 1 of 6 files at r4, all commit messages.
Reviewable status: 14 of 19 files reviewed, 3 unresolved discussions (waiting on @SKairinos)
src/pages/teacherOnboarding/CreateSchoolForm.tsx
line 30 at r3 (raw file):
Previously, SKairinos (Stefan Kairinos) wrote…
Changes are in the BE.
Still seems required 😞
src/components/form/CreateClassForm.tsx
line 44 at r4 (raw file):
read_classmates_data: false, }} useMutation={useCreateClassMutation}
I purposefully tried submitting this form incorrectly (by trying to create 2 students with the same name), and it showed me the error message as expected but it also redirected me to the first step of the onboarding? 🤔
Now I seem stuck because I can't create a new school since I'm already in one. I can get out of it by refreshing but then it redirects me to the dashboard, and doesn't finish the onboarding process properly.
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.
Reviewable status: 14 of 19 files reviewed, 3 unresolved discussions (waiting on @faucomte97)
src/pages/teacherOnboarding/CreateSchoolForm.tsx
line 30 at r3 (raw file):
pull BE changes
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.
Reviewable status: 14 of 19 files reviewed, 3 unresolved discussions (waiting on @SKairinos)
src/pages/teacherOnboarding/CreateSchoolForm.tsx
line 30 at r3 (raw file):
Previously, SKairinos (Stefan Kairinos) wrote…
pull BE changes
I have, I'm on main
, pulled the latest and hard installed
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.
Reviewable status: 11 of 19 files reviewed, 3 unresolved discussions (waiting on @faucomte97)
src/components/form/CreateClassForm.tsx
line 44 at r4 (raw file):
Previously, faucomte97 (Florian Aucomte) wrote…
I purposefully tried submitting this form incorrectly (by trying to create 2 students with the same name), and it showed me the error message as expected but it also redirected me to the first step of the onboarding? 🤔
Now I seem stuck because I can't create a new school since I'm already in one. I can get out of it by refreshing but then it redirects me to the dashboard, and doesn't finish the onboarding process properly.
Done. This error flow no longer happens.
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.
Reviewable status: 11 of 19 files reviewed, 3 unresolved discussions (waiting on @faucomte97)
src/pages/teacherOnboarding/CreateSchoolForm.tsx
line 30 at r3 (raw file):
Previously, faucomte97 (Florian Aucomte) wrote…
I have, I'm on
main
, pulled the latest and hard installed
try again
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.
Reviewed 5 of 6 files at r4, 1 of 3 files at r5, 2 of 2 files at r6, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @SKairinos)
src/pages/teacherOnboarding/StudentCredentialsTable.tsx
line 16 at r1 (raw file):
Previously, SKairinos (Stefan Kairinos) wrote…
Done.
The text is still missing.
Also, we can input any characters in the field, but let's make that a separate task to sanitise input across all text fields.
src/pages/teacherOnboarding/TeacherOnboarding.tsx
line 108 at r6 (raw file):
/> {activeStep.classId && activeStep.students && ( <PrintStudentCredentialsNotification
Please can you also add this notification to the student credentials table page when adding student or resetting student credentials through the Edit Class page - it doesn't seem to show up there. Maybe it's just about moving this code to the StudentCredentials
component, as it should always show there
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.
Reviewable status: 17 of 21 files reviewed, 2 unresolved discussions (waiting on @faucomte97)
src/pages/teacherOnboarding/StudentCredentialsTable.tsx
line 16 at r1 (raw file):
Previously, faucomte97 (Florian Aucomte) wrote…
The text is still missing.
Also, we can input any characters in the field, but let's make that a separate task to sanitise input across all text fields.
Done. Here's the ticket.
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.
Reviewable status: 17 of 21 files reviewed, 2 unresolved discussions (waiting on @faucomte97)
src/pages/teacherOnboarding/TeacherOnboarding.tsx
line 108 at r6 (raw file):
Previously, faucomte97 (Florian Aucomte) wrote…
Please can you also add this notification to the student credentials table page when adding student or resetting student credentials through the Edit Class page - it doesn't seem to show up there. Maybe it's just about moving this code to the
StudentCredentials
component, as it should always show there
Done.
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.
Reviewed 3 of 4 files at r7, 1 of 1 files at r8, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @SKairinos)
🎉 This PR is included in version 1.1.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
This change is