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.

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

Ports - Amy W #37

Open
wants to merge 23 commits into
base: master
Choose a base branch
from
Open

Ports - Amy W #37

wants to merge 23 commits into from

Conversation

amythetester
Copy link

Task List

Congratulations! You're submitting your assignment!

Comprehension Questions

Question Answer
Describe in your own words what the Model is doing in Rails This handles the data and business logic.
Describe in your own words what the Controller is doing in Rails This receives the request from the browser, via the router, takes appropriate action in the model and displays the right page via the views.
Describe in your own words what the View is doing in Rails Handles the visual logic.
Describe an edge-case controller test you wrote When invalid ids are used for tasks.
What is the purpose of using strong params? (i.e. the params method in the controller) Security and ensuring only the params you intend to be used are.
How are Rails migrations related to Rails models? Migrations help make changes to specific tables.
Describe one area of Rails that you are still unclear on When to use initialize variables and local variables in the model.

@tildeee
Copy link

tildeee commented Apr 22, 2019

Task List

What We're Looking For

Feature Feedback
Baseline
Appropriate Git Usage with no extraneous files checked in x
Answered comprehension questions x
Successfully handles: Index, Show x
Index & Show tests pass x
Successfully handles: New, Create x
New & Create tests pass x
Successfully handles: Edit, Update x
Tests for Edit & Update test valid & invalid task ids x
Successfully handles: Destroy, Task Complete x
Tests for Destroy & Task Complete include tests for valid and invalid task ids x
Routes follow RESTful conventions x
Uses named routes (like _path) x
Overall

Great work on this project, Amy! This project looks great; it meets the requirements on using Rails best practices, RESTful routing, and CRUD operations.

One place that this could improve would be refactoring things such that the form is pulled into a partial views. I also have a few comments on the code.

There is actually an interesting bug in your project. To recreate:

  1. Go to the new task form and fill in the name "Task A" and description "Task A Description"
  2. Press add task
  3. Observe that on the main index page, Task A has the link titled "Unmark Complete" date. I expected this to be "Mark Complete"

I have a comment that shows why this bug happens

Otherwise, well done! Good work

@@ -0,0 +1,86 @@
class TasksController < ApplicationController
Copy link

Choose a reason for hiding this comment

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

This controller is named correctly, but weirdly enough, I'd expect your file name to be tasks_controller.rb, not capitalized like how it currently is (Tasks_controller)

description: params["task"]["description"],
complete: params["task"]["complete"],
complete_date: params["task"]["complete_date"],
)
Copy link

Choose a reason for hiding this comment

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

I'd probably refactor this to use the strong params, so Task.new(task_params) here instead of this hash

COMPLETION DATE: <%= task.complete_date %>
<br>
<%= link_to "View Task", task_path(task.id) %>
<% if task.complete == false %>
Copy link

Choose a reason for hiding this comment

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

Here, you are checking specifically if task.complete is equal to false. However, the default value for task.complete for a new task ends up being nil. Therefore, this conditional ends up incorrectly displaying

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.

2 participants