Skip to content
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.

Sign up for GitHub

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

Jump to bottom

School Info Confirmation Dialog - fetch school name for signed in user #27742

Merged
merged 6 commits into from
Apr 9, 2019
Merged

School Info Confirmation Dialog - fetch school name for signed in user #27742

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
bf86fc8
add api end point
nkiruka Mar 28, 2019
763dba5
add controller method to fetch school name
nkiruka Mar 28, 2019
f4704e0
update component with api endpoint
nkiruka Mar 28, 2019
947e03d
fix undefined error
nkiruka Mar 28, 2019
e16482a
add default props
nkiruka Mar 28, 2019
def9e85
remove console log
nkiruka Mar 29, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
25 changes: 23 additions & 2 deletions apps/src/templates/SchoolInfoConfirmationDialog.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,24 +21,45 @@ class SchoolInfoConfirmationDialog extends Component {

constructor(props) {
super(props);

this.state = {
showSchoolInterstitial: false,
schoolName: props.schoolName,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be required? (Github won't let me put this comment comment in propTypes)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. Thanks!

isOpen: true
};
}

static defaultProps = {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do these defaultProps get used?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@epeach The default props will be used unless the parent component over-rides them.

onUpdate: () => {},
schoolName: ''
};

componentDidMount() {
const {schoolName} = this.state;
if (!schoolName && schoolName.length > 0) {
fetch('/dashboardapi/v1/users/me/school_name')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be supplied by the controller?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is supplied by the controller. I added a controller method to get the school name

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry if I've just forgotten a past PR but how do we decide when to show this dialog again? Where is that code?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

.then(response => response.json())
.then(data => {
this.setState({
schoolName: data.school_name
});
})
.catch(error => this.setState({error}));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not familiar with this setState syntax - to what is the error assigned?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the request to fetch schoolName is unsuccessful, an error is thrown to activate the catch call.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@nkiruka nkiruka Apr 9, 2019 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see, it's the local state of the component

}
}

handleUpdateClick = () => {
this.setState({showSchoolInterstitial: true});
};

renderInitialContent() {
const {onConfirm} = this.props;
const {schoolName} = this.state;
return (
<Body>
<div>
<p>{i18n.schoolInfoDialogDescription()} Lincoln Elementary School?</p>
<p>
{i18n.schoolInfoDialogDescription()} {schoolName}
</p>
</div>
<Button
text={i18n.schoolInfoDialogUpdate()}
Expand Down
7 changes: 7 additions & 0 deletions dashboard/app/controllers/api/v1/users_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,13 @@ def load_user
@user = current_user
end

# GET /api/v1/users/<user_id>/school_name
def get_school_name
render json: {
school_name: current_user&.school
}
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


# GET /api/v1/users/<user_id>/contact_details
def get_contact_details
render json: {
Expand Down
1 change: 1 addition & 0 deletions dashboard/config/routes.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -587,6 +587,7 @@
post 'users/:user_id/using_text_mode', to: 'users#post_using_text_mode'
get 'users/:user_id/using_text_mode', to: 'users#get_using_text_mode'
get 'users/:user_id/contact_details', to: 'users#get_contact_details'
get 'users/:user_id/school_name', to: 'users#get_school_name'

post 'users/:user_id/post_ui_tip_dismissed', to: 'users#post_ui_tip_dismissed'

Expand Down