diff --git a/README.md b/README.md index 8604b43..c22a312 100644 --- a/README.md +++ b/README.md @@ -1,6 +1,6 @@ # Coding Exercise -Hello, _______________! +Hello, Lehman Black! Below is a coding exercise that we believe will allow you to show off your amazing development skills! @@ -76,6 +76,139 @@ The data will be displayed in a sortable table. You will need to determine the type of data in the CSV file based on the headers in the first row. Your program will output a list of Groups, and for each Group, a list of active People in that Group. +### Validating the changes +#### Running +You will need an empty MySQL database to run the example. I used docker to spin up +a server (I'm using version 5.7 due to the root account not working out of the box in +newer container versions) + +``` +docker run --name breezedb -e MYSQL_ROOT_PASSWORD=root -e MYSQL_DATABASE=laravel -p3306:3306 mysql:5.7 +``` + +I also had to install the php-sqlite3 module to get the tests to run. +The installation of `yarn` was missing from my system too, so I had to use `npm` + +``` +npm install -g yarn +``` + +After that I could follow the set up instructions above without any issues. +1. copy `.env.example` to .env and add database information +2. run `composer install` +3. run `yarn install` +4. run `php artisan key:generate && php artisan migrate` +4. start API using `php artisan serve` +5. start react with `yarn start` + +#### Changes Implemented +Added the API endpoint for `api/groups` and supports all the normal CRUD operations. + +The `group_name` column has been added to the `people` table. This can be any string +since it is not checked with the `groups`. In production this should be a foreign key. +This column is also displayed with the other `person` data in the React application. + +Automated testing has been added for the People and Groups APIs. They can be run with +``` +php artisan test +``` + +`curl` can be used to manually check API endpoints. The available routes are: + +``` +# php artisan route:list + ++--------+-----------+--------------------------+----------------+-----------------------------------------------+------------+ +| Domain | Method | URI | Name | Action | Middleware | ++--------+-----------+--------------------------+----------------+-----------------------------------------------+------------+ +| | GET|HEAD | api/groups | groups.index | App\Http\Controllers\GroupsController@index | api | +| | POST | api/groups | groups.store | App\Http\Controllers\GroupsController@store | api | +| | GET|HEAD | api/groups/create | groups.create | App\Http\Controllers\GroupsController@create | api | +| | GET|HEAD | api/groups/{group} | groups.show | App\Http\Controllers\GroupsController@show | api | +| | PUT|PATCH | api/groups/{group} | groups.update | App\Http\Controllers\GroupsController@update | api | +| | DELETE | api/groups/{group} | groups.destroy | App\Http\Controllers\GroupsController@destroy | api | +| | GET|HEAD | api/groups/{group}/edit | groups.edit | App\Http\Controllers\GroupsController@edit | api | +| | GET|HEAD | api/people | people.index | App\Http\Controllers\PeopleController@index | api | +| | POST | api/people | people.store | App\Http\Controllers\PeopleController@store | api | +| | GET|HEAD | api/people/create | people.create | App\Http\Controllers\PeopleController@create | api | +| | GET|HEAD | api/people/{person} | people.show | App\Http\Controllers\PeopleController@show | api | +| | PUT|PATCH | api/people/{person} | people.update | App\Http\Controllers\PeopleController@update | api | +| | DELETE | api/people/{person} | people.destroy | App\Http\Controllers\PeopleController@destroy | api | +| | GET|HEAD | api/people/{person}/edit | people.edit | App\Http\Controllers\PeopleController@edit | api | ++--------+-----------+--------------------------+----------------+-----------------------------------------------+------------+ +``` + +The React application now has a place at the top to upload CSV files. The type of +resource is automatically detected. If the CSV file is not a recognized resource +then nothing happens when you click the upload button (see the tasks remaining section) + +There is some console logs for when the upload finishes, but nothing displays on the screen. +You must also refresh the page to view the data changes + +The CSV files do not depend on each other so you can upload them in any order. +(benefit of using a string for the `person.group_name`. In production, I would +create any missing groups or default missing groups to an empty string) + +Sample CSV files can be found in `tests/sample_data`. + + +#### CSV Format +People +``` +id,first_name,last_name,email_address,status,group_name +1,"Alex","Ortiz-Rosado","alex@breezechms.com",active,"Bible Study" +2,"Jon","VerLee","jon@breezechms.com","archived","Bible Study" +3,"Fred","Flintstone","fredflintstone@example.com","active","Bible Study" +4,"Marie","Bourne","mbourne@example.com","active","Vacation" +5,"Wilma","Flintstone","wilmaflinstone@example.com","active","Elders" +``` + +Groups +``` +id,group_name +1,Volunteers +2,Elders +3,"Bible Study" +``` + +#### Known Issues +* **columns are not sortable. ...It's just a matter of adding the right `sortable` state as given in + https://react.semantic-ui.com/collections/table/#variations-sortable** +* `id` is required, but does not quite work as expected. It is an autoincrementing +column so new ID's may not match if you skip numbers or do not have the IDs in a sorted order +* `jest` tests for ResultsList is broken due to updates. Should just need to update the expected +data with the new JSX changes +* have not checked for how large a CSV can be processed +* uploading multiple files without refreshing didn't work during testing +* The PapaParse JavaScript library can be tweaked to be more friendly, but is kind of strict +* Some console errors due to 204 No Content responses from the API being handled improperly +using the defaults. I have noticed issues with the following: + * Columns should not have spaces between them. (Bad: `id, group_name`, Good: `id,group_name` ) + * PapaParse doesn't allow you set multiple quote characters, so I set it to use `"` + +#### Tasks Remaining +* UI feedback + * Add dimmer to show files are uploading and prevent multiple uploads accidently + * add upload progress (PapaParse lets you have a callback for each row / chain state updates to the fetch promises) + * notify user when upload finishes + * show results of the upload (created, updated, errors) + * add link in upload results to show error details + * automatically refresh tables + * allow user to download sample files + * notify user if no file selected or bad file selected + * breeze demo upload uses popup for uploads, would style to match production conventions +* better error handling +* abstract `PeopleService` and `GroupService` into a factory. All that really changes is the URL, +and the allowed fields +* verify CSRF or other XSS protection beyond React's defaults are needed +* If performance became an issue then the CSV parsing and API requests could be done in batches +with some modifications to the API POST/PUT endpoints. +* Update JS to match coding standards (current company does not use ES6+ so I'm a bit rusty) +* Add missing code documentation + + + + ### Testing We love TDD! So we’d love to see tests for the API and ReactJS application. Write automated tests to verify your results and account for gotchas (handling different column orders, invalid id's in the People CSV file, etc..). Classify your tests as either unit, integration, ui, or acceptance, but it is not required to use every type. diff --git a/app/Http/Controllers/GroupsController.php b/app/Http/Controllers/GroupsController.php new file mode 100644 index 0000000..31261c3 --- /dev/null +++ b/app/Http/Controllers/GroupsController.php @@ -0,0 +1,103 @@ +validate([ + 'group_name' => 'required|max:255', + ]); + + $group = Group::create($request->all()); + + return (new GroupResource($group)) + ->response() + ->setStatusCode(201); + } + + /** + * Display the specified resource. + * + * @param int $id + * @return \Illuminate\Http\Response + */ + public function show($id) + { + return new GroupResource(Group::findOrFail($id)); + } + + /** + * Show the form for editing the specified resource. + * + * @param int $id + * @return \Illuminate\Http\Response + */ + public function edit($id) + { + // + } + + /** + * Update the specified resource in storage. + * + * @param \Illuminate\Http\Request $request + * @param int $id + * @return \Illuminate\Http\Response + */ + public function update(Request $request, $id) + { + $group = Group::findOrFail($id); + $group->update($request->all()); + + return response()->json(null, 204); + } + + /** + * Remove the specified resource from storage. + * + * @param int $id + * @return \Illuminate\Http\Response + */ + public function destroy($id) + { + $group = Group::findOrFail($id); + $group->delete(); + + return response()->json(null, 204); + } +} diff --git a/app/Http/Resources/GroupResource.php b/app/Http/Resources/GroupResource.php new file mode 100644 index 0000000..9201a89 --- /dev/null +++ b/app/Http/Resources/GroupResource.php @@ -0,0 +1,24 @@ + $this->id, + 'group_name' => $this->group_name, + 'created_at' => $this->created_at, + 'updated_at' => $this->updated_at, + ]; + } +} diff --git a/app/Http/Resources/GroupsCollection.php b/app/Http/Resources/GroupsCollection.php new file mode 100644 index 0000000..b7b41b3 --- /dev/null +++ b/app/Http/Resources/GroupsCollection.php @@ -0,0 +1,21 @@ + $this->collection + ]; + } +} diff --git a/app/Http/Resources/PersonResource.php b/app/Http/Resources/PersonResource.php index 885b36d..02228fe 100644 --- a/app/Http/Resources/PersonResource.php +++ b/app/Http/Resources/PersonResource.php @@ -20,6 +20,7 @@ public function toArray($request) 'last_name' => $this->last_name, 'email_address' => $this->email_address, 'status' => $this->status, + 'group_name' => $this->group_name, 'created_at' => $this->created_at, 'updated_at' => $this->updated_at, ]; diff --git a/app/Models/Group.php b/app/Models/Group.php new file mode 100644 index 0000000..ef3534e --- /dev/null +++ b/app/Models/Group.php @@ -0,0 +1,12 @@ +define(Group::class, function (Faker $faker) { + return [ + 'group_name' => $faker->name, + ]; +}); diff --git a/database/factories/PersonFactory.php b/database/factories/PersonFactory.php index ccb9195..66987fe 100644 --- a/database/factories/PersonFactory.php +++ b/database/factories/PersonFactory.php @@ -10,6 +10,7 @@ 'first_name' => $faker->firstName, 'last_name' => $faker->lastName, 'email_address' => $faker->email, - 'status' => (bool)random_int(0, 1) ? 'active' : 'archived' + 'status' => (bool)random_int(0, 1) ? 'active' : 'archived', + 'group_name' => $faker->name, ]; }); diff --git a/database/migrations/2020_06_28_222744_create_groups_table.php b/database/migrations/2020_06_28_222744_create_groups_table.php new file mode 100644 index 0000000..3960b33 --- /dev/null +++ b/database/migrations/2020_06_28_222744_create_groups_table.php @@ -0,0 +1,34 @@ +bigIncrements('id'); + $table->string('group_name'); + $table->timestamps(); + }); + }); + } + + /** + * Reverse the migrations. + * + * @return void + */ + public function down() + { + Schema::dropIfExists('groups'); + } +} diff --git a/database/migrations/2020_06_29_031153_add_group_name_to_people_table.php b/database/migrations/2020_06_29_031153_add_group_name_to_people_table.php new file mode 100644 index 0000000..6387ae9 --- /dev/null +++ b/database/migrations/2020_06_29_031153_add_group_name_to_people_table.php @@ -0,0 +1,33 @@ +string('group_name')->default(''); + // + }); + } + + /** + * Reverse the migrations. + * + * @return void + */ + public function down() + { + Schema::table('people', function (Blueprint $table) { + $table->dropColumn('group_name'); + }); + } +} diff --git a/package.json b/package.json index 2761705..e124b07 100644 --- a/package.json +++ b/package.json @@ -5,11 +5,14 @@ "dependencies": { "enzyme": "^3.10.0", "enzyme-adapter-react-16": "^1.14.0", + "lodash": "^4.17.15", + "papaparse": "^5.2.0", "react": "^16.8.6", "react-dom": "^16.8.6", "react-scripts": "3.0.1", "react-test-renderer": "^16.8.6", - "semantic-ui-react": "^0.87.3" + "semantic-ui-react": "^0.87.3", + "yarn": "^1.22.4" }, "scripts": { "start": "react-scripts start", diff --git a/routes/api.php b/routes/api.php index ba2d16a..49335e7 100644 --- a/routes/api.php +++ b/routes/api.php @@ -14,3 +14,4 @@ */ Route::resource('people', 'PeopleController'); +Route::resource('groups', 'GroupsController'); diff --git a/src/CsvUpload.js b/src/CsvUpload.js new file mode 100644 index 0000000..711a157 --- /dev/null +++ b/src/CsvUpload.js @@ -0,0 +1,35 @@ +import React, { Component } from 'react' +import { Button, Header, Input, Segment } from 'semantic-ui-react' +import CsvUploadService from "./CsvUploadService"; + +class CsvUpload extends Component { + constructor(props) { + super(props); + this.myRef = React.createRef(); + this.handleClick = this.handleClick.bind(this); + this.getFileInputNode = this.getFileInputNode.bind(this); + this.state = {}; + } + getFileInputNode() { + return this.myRef.current.inputRef.current; + } + + handleClick() { + let files = this.getFileInputNode().files; + + if (files[0]) { + CsvUploadService.upload(files[0]); + } + } + + render() { + return ( + +
Upload CSV File
+ Upload}/> + + ); + } +} + +export default CsvUpload; diff --git a/src/CsvUploadService.js b/src/CsvUploadService.js new file mode 100644 index 0000000..c8ab583 --- /dev/null +++ b/src/CsvUploadService.js @@ -0,0 +1,54 @@ +import Papaparse from 'papaparse'; +import PeopleService from "./PeopleService"; +import GroupsService from "./GroupsService"; + +const enabledServices = [PeopleService, GroupsService]; + +/** + * @see https://www.papaparse.com/docs#config + */ +var defaults = { + complete: onParseComplete, + dynamicTyping: true, + error: onFileReaderError, + header: true, + quoteChar: '"', + skipEmptyLines: 'greedy', + worker: true, +} + +export default { + upload: upload, +}; + +function upload(file, config) { + if (!config) { + config = defaults; + } + + Papaparse.parse(file, config) +} + +function getCsvService(fields) { + for (let i = 0; i < enabledServices.length; i++) { + if (enabledServices[i].canProcessCsv(fields)) { + console.log(enabledServices[i]); + return enabledServices[i]; + } + } + return null; +} + +function onParseComplete(results, file) { + let fields = results.meta.fields; + let service = getCsvService(fields); + if (!service) { + return; // TODO handle unknown CSV format + } + + service.process(results.data); +} + +function onFileReaderError(error, file) { + console.error('file error in %s: %s', file, error); +} diff --git a/src/GroupsService.js b/src/GroupsService.js new file mode 100644 index 0000000..11b371c --- /dev/null +++ b/src/GroupsService.js @@ -0,0 +1,59 @@ +import _ from 'lodash'; +export default { + canProcessCsv: canProcessCsv, + process: process, + create: create, + update: update, + createOrUpdate: createOrUpdate +}; + +function canProcessCsv(fields) { + return _.isEqual(fields, ['id', 'group_name']); +} + +function process(data) { + Promise.all(data.map(createOrUpdate)).then(() => { + console.log('all people uploaded'); + }); + console.log(data); +} + +function update(group) { + return fetch("http://localhost:8000/api/groups/" + group.id, { + method: 'PUT', headers: { + 'Content-Type': 'application/json', + 'Accept': 'application/json' + }, + body: JSON.stringify(group) + }); +} + +function create(group) { + return fetch("http://localhost:8000/api/groups", { + method: 'POST', + headers: { + 'Content-Type': 'application/json', + 'Accept': 'application/json' + }, + body: JSON.stringify(group) + }); +} + +function createOrUpdate(group) { + update(group) + .then(response => { + console.debug(response); + if (response.status == 404) { + console.log('creating group %o', group); + return create(group); + } else if (!response.ok) { + console.error('something went wrong creating group: %o' + group); + return Promise.reject('something went wrong creating group: ' + JSON.stringify(group)); + } else { + let group = response.json(); + console.log('updated group %o', group); + console.debug(group); + return group; + } + }); +} diff --git a/src/PeopleService.js b/src/PeopleService.js new file mode 100644 index 0000000..4f6f683 --- /dev/null +++ b/src/PeopleService.js @@ -0,0 +1,59 @@ +import _ from 'lodash'; +export default { + canProcessCsv: canProcessCsv, + process: process, + create: create, + update: update, + createOrUpdate: createOrUpdate +}; + +function canProcessCsv(fields) { + return _.isEqual(fields, ['id', 'first_name', 'last_name', 'email_address', 'status', 'group_name']); +} + +function process(data) { + Promise.all(data.map(createOrUpdate)).then(() => { + console.log('all people uploaded'); + }); + console.log(data); +} + +function update(person) { + return fetch("http://localhost:8000/api/people/" + person.id, { + method: 'PUT', headers: { + 'Content-Type': 'application/json', + 'Accept': 'application/json' + }, + body: JSON.stringify(person) + }); +} + +function create(person) { + return fetch("http://localhost:8000/api/people", { + method: 'POST', + headers: { + 'Content-Type': 'application/json', + 'Accept': 'application/json' + }, + body: JSON.stringify(person) + }); +} + +function createOrUpdate(person) { + update(person) + .then(response => { + console.debug(response); + if (response.status == 404) { + console.log('creating person %o', person); + return create(person); + } else if (!response.ok) { + console.error('something went wrong creating person: %o' + person); + return Promise.reject('something went wrong creating person: ' + JSON.stringify(person)); + } else { + let person = response.json(); + console.log('updated person %o', person); + console.debug(person); + return person; + } + }); +} diff --git a/src/ResultsList.js b/src/ResultsList.js index 3367c82..666b23b 100644 --- a/src/ResultsList.js +++ b/src/ResultsList.js @@ -1,50 +1,100 @@ +import _ from 'lodash'; import React, { Component } from 'react' -import { Table } from 'semantic-ui-react' +import {Divider, Header, Segment, Table} from 'semantic-ui-react' class ResultsList extends Component { constructor(props) { super(props); - this.state = { data: [] }; + this.state = { + data: [], + group_data: [], + }; } componentDidMount() { fetch("http://localhost:8000/api/people") .then(response => response.json()) .then(data => this.setState({ data: data.data })); + + fetch("http://localhost:8000/api/groups") + .then(response => response.json()) + .then(data => this.setState({ group_data: data.data })); } render() { var data = this.state.data || []; + var group_data = this.state.group_data || []; - return ( - - - - First Name - Last Name - Email - Status - - - - - - { - data.map((person, index) => { - return ( - - { person.first_name } - { person.last_name } - { person.email_address } - { person.status } - - ); - }) - } + return ( +
+
People
+
+ + + First Name + Last Name + Email + Group + Status + + + + - -
- ); + { + data.map((person, index) => { + return ( + + { person.first_name } + { person.last_name } + { person.email_address } + { person.group_name } + { person.status } + + ); + }) + } + + + + +
Groups
+ { + group_data.map((group, index) => { + return ( + +
{group.group_name}
+ + + + First Name + Last Name + Email + Status + + + + + { + _.filter(data, {status: 'active', group_name: group.group_name}).map((person, index) => { + return ( + + { person.first_name } + { person.last_name } + { person.email_address } + { person.status } + + ); + }) + } + +
+ + ); + }) + } + + ); } } diff --git a/src/index.js b/src/index.js index 4ab037e..0313080 100644 --- a/src/index.js +++ b/src/index.js @@ -3,6 +3,7 @@ import ReactDOM from "react-dom"; import { Container, Header } from "semantic-ui-react"; import ResultsList from "./ResultsList"; +import CsvUpload from "./CsvUpload"; const App = ({ children }) => ( @@ -19,6 +20,7 @@ document.head.appendChild(styleLink); ReactDOM.render( + , document.getElementById("root") diff --git a/tests/Api/GroupTest.php b/tests/Api/GroupTest.php new file mode 100644 index 0000000..876a04f --- /dev/null +++ b/tests/Api/GroupTest.php @@ -0,0 +1,86 @@ + 'Bible Study', + ]; + $response = $this->json('POST', '/api/groups', $expected); + $response + ->assertStatus(201) + ->assertJsonFragment($expected); + + } + + public function testGroupRetrieved() + { + $group = factory('App\Models\Group')->create(); + + $response = $this->json('GET', '/api/groups/' . $group->id); + $response + ->assertStatus(200) + ->assertJsonStructure([ + 'data' => [ + 'group_name', + 'created_at', + 'updated_at' + ] + ]); + } + + public function testAllGroupsRetrieved() + { + $group = factory('App\Models\Group', 25)->create(); + + $response = $this->json('GET', '/api/groups'); + $response + ->assertStatus(200) + ->assertJsonCount(25, 'data'); + } + + public function testNoGroupRetrieved() + { + $group = factory('App\Models\Group')->create(); + Group::destroy($group->id); + + $response = $this->json('GET', '/api/groups/' . $group->id); + $response->assertStatus(404); + } + + public function testGroupUpdated() + { + $group = factory('App\Models\Group')->create(); + + $updatedGroupName = $this->faker->name(); + $response = $this->json('PUT', '/api/groups/' . $group->id, [ + 'group_name' => $updatedGroupName + ]); + $response->assertStatus(204); + + $updatedGroup = Group::find($group->id); + $this->assertEquals($updatedGroupName, $updatedGroup->group_name); + } + + public function testGroupDeleted() + { + $group = factory('App\Models\Group')->create(); + + $deleteResponse = $this->json('DELETE', '/api/groups/' . $group->id); + $deleteResponse->assertStatus(204); + + $response = $this->json('GET', '/api/groups/' . $group->id); + $response->assertStatus(404); + + } +} diff --git a/tests/Api/PeopleTest.php b/tests/Api/PeopleTest.php index d5416f5..d5edd90 100644 --- a/tests/Api/PeopleTest.php +++ b/tests/Api/PeopleTest.php @@ -17,7 +17,8 @@ public function testPersonCreated() 'first_name' => 'Sally', 'last_name' => 'Ride', 'email_address' => 'sallyride@nasa.gov', - 'status' => 'archived' + 'status' => 'archived', + 'group_name' => 'Example Group', ]; $response = $this->json('POST', '/api/people', $expected); $response @@ -39,6 +40,7 @@ public function testPersonRetrieved() 'last_name', 'email_address', 'status', + 'group_name', 'created_at', 'updated_at' ] diff --git a/tests/sample_data/example-group-update.csv b/tests/sample_data/example-group-update.csv new file mode 100644 index 0000000..ce9362c --- /dev/null +++ b/tests/sample_data/example-group-update.csv @@ -0,0 +1,4 @@ +id,group_name +1,"Volunteers Update" +2,"Elders asdf" +3,"Bible Study ddddd" \ No newline at end of file diff --git a/tests/sample_data/example-group.csv b/tests/sample_data/example-group.csv new file mode 100644 index 0000000..136c9bf --- /dev/null +++ b/tests/sample_data/example-group.csv @@ -0,0 +1,4 @@ +id,group_name +1,"Volunteers" +2,"Elders" +3,"Bible Study" \ No newline at end of file diff --git a/tests/sample_data/people-with-groups.csv b/tests/sample_data/people-with-groups.csv new file mode 100644 index 0000000..bf2395e --- /dev/null +++ b/tests/sample_data/people-with-groups.csv @@ -0,0 +1,6 @@ +id,first_name,last_name,email_address,status,group_name +1,"Alex","Ortiz-Rosado","alex@breezechms.com",active,"Bible Study" +2,"Jon","VerLee","jon@breezechms.com","archived","Bible Study" +3,"Fred","Flintstone","fredflintstone@example.com","active","Bible Study" +4,"Marie","Bourne","mbourne@example.com","active","Vacation" +5,"Wilma","Flintstone","wilmaflinstone@example.com","active","Misc" \ No newline at end of file diff --git a/yarn.lock b/yarn.lock index f8ffcd6..4c7299f 100644 --- a/yarn.lock +++ b/yarn.lock @@ -10358,3 +10358,8 @@ yargs@^13.3.0: which-module "^2.0.0" y18n "^4.0.0" yargs-parser "^13.1.1" + +yarn@^1.22.4: + version "1.22.4" + resolved "https://registry.yarnpkg.com/yarn/-/yarn-1.22.4.tgz#01c1197ca5b27f21edc8bc472cd4c8ce0e5a470e" + integrity sha512-oYM7hi/lIWm9bCoDMEWgffW8aiNZXCWeZ1/tGy0DWrN6vmzjCXIKu2Y21o8DYVBUtiktwKcNoxyGl/2iKLUNGA==