Jamila Octos #24
Jamila Octos #24
Conversation
…d be replaced by LF error
Media RankerWhat We're Looking For
This is a good start, but there's a lot missing from this assignment. Most of the core functionality of the site (CRUD on works, logging in, voting) is there, which is no small thing. However, I'm worried the following learning goals were missed:
The first two are around building a cohesive user experience. Since you the developer are also the first user of the site, putting at least some work into this early is often good for both morale and dev velocity. For example, always reporting validation failures whenever you build a form can save you from spending a bunch of time chasing down a "bug" that was really just a typo in your input. Testing is a difficult skill to master but it's also an essential one, both because it allows you to check that your code is doing the right thing, and because it forces you to step back from your code and think about it from the outside, asking questions like
Being able to reason about your code in this way will make you a much stronger engineer, which is why testing is such an important part of our curriculum. Fortunately, all of these are things you'll have ample opportunity to practice on bEtsy - please make sure you're finding opportunities to work on them. For example, you should go through right now and make sure that all your site's forms are reporting validation errors to the user. If you need early feedback on test cases I'd be happy to provide it. I know you've been putting in a lot of hours recently. Keep up that hard work and dedication, and you'll get to where you need to be. |
| resources :users | ||
|
|
||
| get '/login', to: 'sessions#new', as: 'login' | ||
| post '/login', to: 'sessions#create' |
There was a problem hiding this comment.
I don't think you need all 7 restful routes for users - only index and show are needed for this project. sessions#new and sessions#create cover the workflow of making a new user, and editing or destroying a user aren't part of the requirements.
| @@ -0,0 +1,14 @@ | |||
| <%= form_for @work do |f| %> | |||
| <%= f.label :work_type %> | |||
| <%= f.text_field :category %> | |||
There was a problem hiding this comment.
Instead of a text box, you should use a dropdown here so the user has no chance of making a mistake.
| if @work.save | ||
| redirect_to work_path(@work.id) | ||
| end | ||
|
|
There was a problem hiding this comment.
The if statement on line 14 doesn't have an else, for if the work failed to save. That means you don't have any way to report validation failures to the user, nor do you give them an opportunity to correct their error.
| resources :votes | ||
|
|
||
| resources :works do | ||
| resources :votes, only: [:create] |
There was a problem hiding this comment.
Do you need any routes for votes other than create?
Also, you should either have a nested or un-nested version of votes#create, not both.
| end | ||
|
|
||
| def update | ||
| user.assign_attributes(user_params) |
There was a problem hiding this comment.
You should only need index and show in this controller, since most of the other user workflows are handled by the SessionsController or not supported at all.
| <% if model.errors.messages.any? %> | ||
| <section class="alert callout"> | ||
| <h3>There were some problems</h3> | ||
| <ul> |
There was a problem hiding this comment.
You've got this partial do display validation errors, but you're only using it for votes. You should use it for users and works as well.
| def self.albums | ||
| Work.where(category: "album") | ||
|
|
||
| end |
There was a problem hiding this comment.
I like that you've isolated these as model methods. That means if you were to, say, order them by vote count, you could do so here without having to change your view code at all.
|
|
||
| def update | ||
| work.assign_attributes(work_params) | ||
|
|
There was a problem hiding this comment.
The update method doesn't work! I suspect you want @work on line 26.
| class User < ApplicationRecord | ||
| has_many :votes | ||
| validates :name, presence: true | ||
| validates :votes, uniqueness: {scope: :work} |
There was a problem hiding this comment.
I had to disable the validations on lines 3 and 4 to log in.
On line 3, I suspect you want to be validating user_name instead of name.
Not sure what's up with line 4, especially since you validate vote uniqueness in the Vote model.
| belongs_to :user | ||
| belongs_to :work | ||
|
|
||
| validates :user, uniqueness: {scope: :work} |
There was a problem hiding this comment.
Good work getting this tricky validation sorted out.
2 participants
Media Ranker
Congratulations! You're submitting your assignment!
Comprehension Questions
sessionandflash? What is the difference between them?