School Info Confirmation Dialog - fetch school name for signed in user #27742
Conversation
| @@ -21,24 +21,46 @@ class SchoolInfoConfirmationDialog extends Component { | |||
|
|
|||
| constructor(props) { | |||
| super(props); | |||
|
|
|||
| this.state = { | |||
| showSchoolInterstitial: false, | |||
| schoolName: props.schoolName, | |||
There was a problem hiding this comment.
Should this be required? (Github won't let me put this comment comment in propTypes)
There was a problem hiding this comment.
@bethanyaconnor Are you referring to schoolName? There might be times that a teacher did not enter the name of the school; so I decided to have a default value of an empty string
There was a problem hiding this comment.
Makes sense. Thanks!
| fetch('/dashboardapi/v1/users/me/school_name') | ||
| .then(response => response.json()) | ||
| .then(data => { | ||
| console.log(data); |
| componentDidMount() { | ||
| const {schoolName} = this.state; | ||
| if (!schoolName && schoolName.length > 0) { | ||
| fetch('/dashboardapi/v1/users/me/school_name') |
There was a problem hiding this comment.
Shouldn't this be supplied by the controller?
There was a problem hiding this comment.
Yes, it is supplied by the controller. I added a controller method to get the school name
There was a problem hiding this comment.
Sorry if I've just forgotten a past PR but how do we decide when to show this dialog again? Where is that code?
There was a problem hiding this comment.
Good question, I sent another PR that includes the code. I added a helper method that contains the logic for when to show the dialog.
| this.state = { | ||
| showSchoolInterstitial: false, | ||
| schoolName: props.schoolName, | ||
| isOpen: true | ||
| }; | ||
| } | ||
|
|
||
| static defaultProps = { |
There was a problem hiding this comment.
@epeach The default props will be used unless the parent component over-rides them.
| schoolName: data.school_name | ||
| }); | ||
| }) | ||
| .catch(error => this.setState({error})); |
There was a problem hiding this comment.
I'm not familiar with this setState syntax - to what is the error assigned?
There was a problem hiding this comment.
If the request to fetch schoolName is unsuccessful, an error is thrown to activate the catch call.
There was a problem hiding this comment.
Sorry - my question should have been clearer! Once the catch call is activated, and this.setState({error}) is called, to which part of the state is error set?
There was a problem hiding this comment.
Ah I see, it's the local state of the component
|
@epeach @bethanyaconnor Thanks for the code review, let me know if you have any additional questions or if this PR is good to go. |
| render json: { | ||
| school_name: current_user&.school | ||
| } | ||
| end |
There was a problem hiding this comment.
This whole controller strikes me as very odd. Our route takes a user_id parameter, but it must be me or the current user's id, and our controller method here ignores the parameter anyway. Something we'd want to clean up in the future? At the very least, I'd suggest this method use @user.school, and have a test that confirms we can only get the current_user's school name, so if the load_user method is changed in the future we know this behavior is preserved.
An api end point was added to fetch the school name for a signed in user (teacher).
Part 2: PR 27534