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 - Jillianne #36

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

Ports - Jillianne #36

wants to merge 23 commits into from

Conversation

jillirami
Copy link

@jillirami jillirami commented Apr 15, 2019

Task List

Congratulations! You're submitting your assignment!

Comprehension Questions

Question Answer
Describe in your own words what the Model is doing in Rails The model is linked to database and contains knowledge about the application, can be reused.
Describe in your own words what the Controller is doing in Rails The controller processes requests and ties in the information of the model and the view for the application's overall function.
Describe in your own words what the View is doing in Rails The view is visible to the user, related to HTML.
Describe an edge-case controller test you wrote If variables passed to the update function were invalid, the expected status code was tested for.
What is the purpose of using strong params? (i.e. the params method in the controller) Strong params prevent the user from directly updating model attributes, specifying the parameters information is passed.
How are Rails migrations related to Rails models? Migrations establish and modify the parameters of the programs model.
Describe one area of Rails that are still unclear on How to correctly use params in code and testing.

@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

Jillianne! Great work with this project! Your code meets all of the requirements I was looking for: Rails best practices, RESTful routing, and CRUD. Particularly, I loooove how well-written your controller tests are!

I only have a few notes for improvement. Otherwise, I think this is a great project submission.

Also, I want to note that your verify route and user experience is really cool, but above and beyond what we asked for! Let me know if you have any questions on that.

Well done!

class TasksController < ApplicationController
# TASKS = [
# {name: "Call Ma", description: "Family First", completed_at: "", completion_date: ""}
# ]
Copy link

Choose a reason for hiding this comment

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

Don't forget to delete unused variables!

# ]

def index
@tasks = Task.all.order(:created_at)
Copy link

Choose a reason for hiding this comment

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

Nice detail on ordering these

def toggle_complete
task = Task.find_by(id: params[:id])

if task.completed_at == "Incomplete"
Copy link

Choose a reason for hiding this comment

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

Instead of assuming that .completed_at will be a string of either "Incomplete" or "Complete", is there a better data type for this?

I think a boolean would be much better, and would prevent bugs. This way you don't need to do if ... elsif ... else, but just if ... else

def toggle_complete
task = Task.find_by(id: params[:id])

if task.completed_at == "Incomplete"
Copy link

Choose a reason for hiding this comment

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

Is completed_at the best name for this attribute if it is describing the state of the task's completion?

@@ -0,0 +1,37 @@
<link href="stylesheets/index.css" rel="stylesheet" type="text/css">
Copy link

Choose a reason for hiding this comment

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

You shouldn't need to do have this line in order to load your CSS. Rails should know how to load its own CSS

<% @tasks.each do |task| %>
<br>
<td><%= task.name %></td>
<% if task.completed_at.nil? %>
Copy link

Choose a reason for hiding this comment

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

You'll probably want to indent the code that's within the if (and also the else)

<br>
<td><%= task.name %></td>
<% if task.completed_at.nil? %>
<br>
Copy link

Choose a reason for hiding this comment

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

Better to not use br tags as they're not semantic

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