Journalist can assign an article to a category #26
Conversation
|
|
||
| categories.each do |category_id| | ||
| category = Category.find_by(id: category_id) | ||
| @article.categories.include?(category) ? next : @article.categories << category |
There was a problem hiding this comment.
Could you please see if this solution work?
@article.categories.push category unless @article.categories.include? categoryfix small bug due to spelling
refactor test for assigning articles to categories
adds show view for categories
holgertidemand
changed the title
There was a problem hiding this comment.
Please see comments. I would also add category to the Article factory. Also, ask yourself if there's a use case for articles WITHOUT a category. If not, please make sure there's a method to make sure there's always a category associated with an article. (validation)
app/controllers/home_controller.rb
Outdated
| @@ -1,24 +1,15 @@ | |||
| class HomeController < ApplicationController | |||
| before_action :get_coordinates, only: [:index] | |||
| GOTHENBURG = [57.700501, 11.975463] | |||
There was a problem hiding this comment.
In what way is this code related to this feature?
There was a problem hiding this comment.
That's is my bad. Local gitflow mistake
app/controllers/home_controller.rb
Outdated
| def get_location | ||
| user = create_guest_user | ||
| current_user ? current_user.address : user.address | ||
| @local_articles = Article.near(current_user.address, 20) if [email protected]? |
There was a problem hiding this comment.
In what way is this code related to this feature?
app/controllers/home_controller.rb
Outdated
| end | ||
|
|
||
| def set_edition | ||
| if User.near([57.700501, 11.975463], 50).include? current_user | ||
| if User.near(GOTHENBURG, 50).include? current_user |
There was a problem hiding this comment.
In what way is this code related to this feature?
app/controllers/home_controller.rb
Outdated
| end | ||
| current_user.latitude = @coordinates.values.first | ||
| current_user.longitude = @coordinates.values.second | ||
| current_user.save |
There was a problem hiding this comment.
In what way is this code refactoring related to this feature?
| article = Article.find_by(title: article[:title]) | ||
| article.categories.push category | ||
| end | ||
| end |
There was a problem hiding this comment.
Why not do this in the step definition where you create articles?
There was a problem hiding this comment.
This made more since to us. didn't know if that would mess up all the other test that are using that step so we decided to make a new one.
| end | ||
|
|
||
| Then("I select {string} from {string}") do |category, select_box| | ||
| select(category, from: select_box) |
There was a problem hiding this comment.
I sense that you are mixing styles. Most of the step definitions omit the parathesis, here you have them. Stick to one style, please.
| Background: | ||
| Given the following article exists | ||
| | title | content | | ||
| | A Whole New World | A new fantastic point of view | |
There was a problem hiding this comment.
Why do you have this background step here?
There was a problem hiding this comment.
Wasn't sure how we would make it write the test in the beginning. Will remove it!
| And the following user exists | ||
| | email | password | | ||
| | [email protected] | OsloOslo123 | | ||
| And the following categories exists |
There was a problem hiding this comment.
Please separate steps in the background with an empty line.
| | name | | ||
| | Fashion | | ||
| | Tech | | ||
| And the following article exists |
There was a problem hiding this comment.
Please separate steps in the background with an empty line.
|
You need to have another team member to review this one. I just did a review and red flagged it. It was a go the other day, so you should have merged it when it was. 😳 |
|
Also, from what I understand, you guys implemented Active Admin, right. So Article creation will be moved to that part of the application, right? |
|
@tochman thank you for your comments. |
PT Story: https://www.pivotaltracker.com/story/show/155903746
Description
Changes proposed in this pull request:
What I have learned working on this feature: