Stories Controller: create separate controller and views for articles by search #14377
Conversation
|
Thank you for opening this PR! We appreciate you! For all pull requests coming from third-party forks we will need to A Forem Team member will review this contribution and get back to |
| rescue_from ArgumentError, with: :bad_request | ||
|
|
||
| def index | ||
| @query = "...searching" |
There was a problem hiding this comment.
I know you moved over this code but now is probably a good time to review it. Why are we assigning a static string to an instance variable? It also doesn't seem to be used in the view.
There may be a legitimate reason for this, I'd like to know myself. Same for @article_index which gets assigned in 3 different controllers but doesn't seem to appear elsewhere in the codebase.
There was a problem hiding this comment.
Those variables bothered me as well. I could not find out why they exist but removing them does not seem to affect the search function and tests do not fail either. However, it did not seem like a good idea to remove variables without knowing how they are used so I let them be 😅
| @@ -0,0 +1,22 @@ | |||
| module Stories | |||
There was a problem hiding this comment.
I'm not fully convinced by the naming here. This is in no way your fault but it was always confusing that the model was called Article while the corresponding controller was named StoriesController. On top of that, we're planning to rename the model to Post eventually. 😅 I'd be tempted to have this in Articles::SearchController instead because it's articles we're searching for.
There was a problem hiding this comment.
My first though was to move it to a new controller/articles folder, however there are three controllers dealing with articles in the controller/stories folder. I assume "stories" to be a set of activities dealing with different types of content (posts, podcasts, pages, organizations etc). There's also find_featured_story in app/services/articles/feeds/large_forem_experimental.rb that deals with featuring articles on users home feed according to user preferences and activity. I found it rather difficult to interpret what the term "stories" stand for. As you say, creating controllers in modules that correspond to the model name seems correct. However, I would like to have some more clarity on what stories is 😅
There was a problem hiding this comment.
It's a problem we had in several areas of our codebase, names didn't agree in several places. For example, what is called Listings in the front end was backed by a model called ClassifiedListing which I eventually ended up renaming. "Stories" is not really a word we use in internal discussions, so it's not really a domain concept. Hence the pending rename to Post. In the meantime, I think Articles::Search would make more sense but I am aware that there are already several article-related controllers in the Stories namespace. I guess we might as well just leave it here.
pr-triage
bot
added
PR: partially-approved
pr-triage
bot
added
PR: reviewed-approved
|
@maestromac I'll let you merge this in case we missed something :D |
pr-triage
bot
added
PR: merged
What type of PR is this? (check all applicable)
Description
As discussed in #2914 , the Stories controller has a lot of responsibilities which need to be divided for ease of debugging and testing. This PR divides the task for displaying articles by search in separate controller
stories/articles_search_controller, views, specs and routes for the same have been updated. No changes have been made to the URL for searching articles.Related Tickets & Documents
Added/updated tests?
spec/requests/stories/articles_search_specfromspec/requests/stories_index_spec.rb