Project 2 Feedback

[cameronjacoby]

Project Strengths

• Excellent use of GitHub - commit messages are frequent and clear, and great use of feature branches and pull requests.
• Very helpful README that's detailed without being too verbose. :+1: on including the steps to run the app locally.
• I know it involved a lot of iterations and refactoring, but the overall user flow came together in a way that's logical and intuitive. I'm so proud! :smile:
• I'm extremely impressed with your implementation of the complex back-end functionality we didn't cover in class, specifically Single Table Inheritance, support for local timezones, and splitting time slots into many different meeting instances.
• Custom error-handling in the meeting class. :100:
• Nice use of the FriendlyId gem to make pretty URLs. Great choice since the main user experience is based on the user's "dashboard" (profile page).
• It was a smart choice to go with the calendar tabs for the first iteration of the front-end UI. Great work integrating back-end and front-end logic to send the correct data into the view using group_by.
• Great use of Bootstrap and jQuery overall to achieve the design and front-end functionality. The site is very responsive, and the interactive elements such as the datepicker add a lot to the experience.
• Good test coverage for the users controller. I especially like that you tested the student and ta contexts.
• Your indentation is AMAZING. :100: :100: :100:

  Growth Opportunities

• File Cleanup: There are a couple folders/files in the root directory that should be deleted or gitignored:
  • Delete README.rdoc since you're using README.md instead.
  • Delete the test folder since you're using rspec rather than Rails's built-in testing framework.
  • .DS_Store should be gitignored.
• GitHub: As mentioned above, your overall use of GitHub was excellent. A couple small improvements:
  • Every team member should get in the habit of making commit messages in the present tense.
  • It's ok to keep merged in branches when you're working with a very small number of feature branches (~ 5), but when you get up to a large number like 27, you should delete them once merged in.

• README Steps to Deploy: Again, great README, but you're missing a couple steps in the "How to Use" section:

  • bundle install
  • create and migrate the database
  • any ENV variables?

  For the ENV variables, you could think about making a .sample_env file outlining which variables are needed. Something like this:

  #
  # .sample_env
  #
  
  S3_BUCKET=YOUR_S3_BUCKET
  S3_ACCESS_KEY_ID=YOUR_S3_ACCESS_KEY_ID

• Instructor Refactor: Eventually refactor tas to instructors starting at the database level and extending throughout your app. It's going to be a lot of code changes, but I think it's worth it so that the routes reflect /instructors/cameron instead of /tas/cameron.
• Meetings Service: The meetings#create, meetings#update, and meetings#destroy actions are getting long, and they handle many different scenarios. This could be a good place to refactor into a service class (see screencast, code example, and extra resources).
• Controller Authorization: Use the authorize method in the meetings controller, students controller, and tas controller to prevent the logged in user from executing certain actions. It looks like you're authorizing some of those routes manually, which isn't very DRY, since you already have the helper method defined. Example here.
• Homepage for Logged In Users The "Get Started" button on homepage still goes to /#signup when the user is signed in, even though the sign up form doesn't show. Maybe you can link this button to the instructor or student dashboard when a user is logged in?
• Instructor Schedule: The weeks seem to be in reverse order on the instructor meeting schedule. It would be awesome if you could show the current week/day by default.
• Reserving a Meeting: A few suggestions on the "reserving a meeting" user flow (for students):
  • The view to reserve a meeting shouldn't have the "Edit Meeting" title. Even though that's technically what's happening on the back-end, that doesn't make sense for the student to see, since "reserving" is more like creating to them.
  • If a student hits "Cancel" from the "reserve meeting" flow, I expect them to be redirected back to the instructor's page to see available meetings again, not back to the student's own profile page.
  • You have very thourough database validations preventing meetings from being reserved in the past, but the UX could be improved. Maybe you could "disable" the "Reserve" button on meetings in the past?
• Google Calendar: The Google Calendar integration is awesome! Right now, the link in Google Calendar shows the time in UTC.
• FORM_FOR: Speaks for itself.
• CSS Separation of Concerns: You have enough CSS code where you should start to separate it out into differnt files rather than putting it all in application.scss. You could create a main.scss file for all shared styles, and then create separate stylesheets for any feature-specific styles.

[cameronjacoby]

@tkhuynh @slnwlf @murcielago17 - Here is your Project 2 feedback from the code review. I will be having 1:1 meetings with you all today and tomorrow to go over the feedback.