Skip to content

Creates the skeleton of a Search for Teachers page. - #4568

Merged
ashercodeorg merged 12 commits into
stagingfrom
searchForTeachers
Oct 15, 2015
Merged

Creates the skeleton of a Search for Teachers page.#4568
ashercodeorg merged 12 commits into
stagingfrom
searchForTeachers

Conversation

@ashercodeorg

Copy link
Copy Markdown
Contributor

Later iterations will add functionality and search features to the page.

Note that this PR began as PR#4331. It is being moved here because of git issues.

@ashercodeorg

Copy link
Copy Markdown
Contributor Author

When reviewing, please confirm that I've done all the necessary authentication stuff to require admin access.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

yes, this is requiring admin access. You can write a unit test, there should be some examples in ReportsControllerTest

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Line contains trailing whitespace

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

we use json style for hashes when possible, eg: {method: 'get'}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

@laurelfan

Copy link
Copy Markdown
Contributor

LGTM, except for some style nits and testing.

If you need to get this out soon for some users and are continue to work on it, feel free to postpone testing for further iterations. I assume the functionality will become more complex then and testing will be more necessary...

ashercodeorg added a commit that referenced this pull request Oct 15, 2015
Creates the skeleton of a Search for Teachers page.
@ashercodeorg
ashercodeorg merged commit f77fb8d into staging Oct 15, 2015
@ashercodeorg
ashercodeorg deleted the searchForTeachers branch October 15, 2015 18:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants