Recommended resources #2
Conversation
…l need to fix code
| @@ -7,6 +7,7 @@ class MomentsController < ApplicationController | |||
| include MomentsFormHelper | |||
| include Shared | |||
| include TagsHelper | |||
| # include PagesConcern | |||
There was a problem hiding this comment.
You can remove the commented out code
| @@ -0,0 +1,66 @@ | |||
| # frozen_string_literal: true | |||
| class ResourceRecommendation | |||
There was a problem hiding this comment.
resources and initialize (used by .new) are the only methods exposed to other classes, so all other methods must be private.
|
|
||
| def match_keywords | ||
| all_resources.each do |resource| | ||
| resource_tags = resource['tags'].map do |tag| |
There was a problem hiding this comment.
When you map something and then flats it, you can replace map with flat_map
https://ruby-doc.org/core-2.7.0/Enumerable.html#method-i-flat_map
| def modify_keywords | ||
| @moment_keywords = @moment_keywords.flatten | ||
| @moment_keywords.each do |keyword| | ||
| keyword.gsub!(%r{([_@#!%()\-=;><,{}\~\[\]\./\?\"\*\^\$\+\-]+)}, '') |
There was a problem hiding this comment.
| keyword.gsub!(%r{([_@#!%()\-=;><,{}\~\[\]\./\?\"\*\^\$\+\-]+)}, '') | |
| keyword.gsub!(/[^a-z]+/i, '') |
Instead of passing several special characters, you can match any character that is not a letter
^ menas not
[a-z] means any letter
+ mens one or more
The i at the end means case-insensitive
This is a kind of a JSbin for Relugar expressions. You can test that regex there:
https://rubular.com/r/8NkkIIHtxUBUxv
There was a problem hiding this comment.
I like your simpler way better, especially since it would cover any special characters not present in my current regex expression. However, if we only allow a-z characters, would that not affect special alphabetical characters such as é or ñ? This project is available in several languages, including Spanish, which could pose a problem for matching words.
There was a problem hiding this comment.
That's a good point. Fortunately Ruby has regex support for Unicode characters, such as [[:alpha:]]
https://ruby-doc.org/core-2.7.0/Regexp.html#class-Regexp-label-Character+Classes
| @@ -4571,7 +4571,7 @@ debug@=3.1.0: | |||
| dependencies: | |||
There was a problem hiding this comment.
This file must be out of the PR. To revert it to the same version on master, you can run:
git checkout origin/master client/yarn.lock
| get_keywords(@moment.categories) | ||
| get_keywords(@moment.moods) | ||
| get_keywords(@moment.strategies) | ||
| @moment_keywords.push(moment_name, moment_why, moment_fix) | ||
| modify_keywords |
There was a problem hiding this comment.
You can move all methods related to extracting keywords from a moment to a separated class. That way the extracting step will be separated from matching. This will make the codebase easier to test and maintain.
[ifmeorg#1713] Refactor to use select method without conditional. Co-Authored-By: Gui Vieira <[email protected]>
| @@ -0,0 +1,45 @@ | |||
|
|
|||
| let(:categories) {[FactoryBot.build(:category, | ||
| name: "category name", | ||
| description: "category description." | ||
| )]} |
There was a problem hiding this comment.
For multiline blocks, the most accepted style is to use do...end
| let(:categories) {[FactoryBot.build(:category, | |
| name: "category name", | |
| description: "category description." | |
| )]} | |
| let(:categories) do | |
| FactoryBot.build([ | |
| :category, | |
| name: "category name", | |
| description: "category description." | |
| )] | |
| end |
| name: "strategy-name", | ||
| description: "strategy-description." | ||
| )]} | ||
| subject(:moment) {FactoryBot.build(:moment, |
There was a problem hiding this comment.
As it refers to a class other than the one being tests (MomentKeywords), we usually use let here
There was a problem hiding this comment.
Oh, that's what you said over Zoom! Sorry, the audio wasn't good at the time. :)
| moods: moods, | ||
| strategies: strategies | ||
| ) } | ||
| let(:keywords) { MomentKeywords.new(moment).extract_keywords } |
There was a problem hiding this comment.
This is the class being tested, so you can use subject here
| let(:keywords) { MomentKeywords.new(moment).extract_keywords } | |
| subject(:keywords) { MomentKeywords.new(moment).extract_keywords } |
| "self care", "text", "tested", "is", "teachers"] | ||
| ) | ||
| end | ||
| end |
| why: "More testing content self-care.", | ||
| fix: "Text tested is @Teachers!!") } | ||
|
|
||
| let(:recommended_resources) { ResourceRecommendation.new(moment).resources } |
There was a problem hiding this comment.
Same here. recommended_resources should use subject and moment use let
| let(:recommended_resources) { ResourceRecommendation.new(moment).resources } | |
| subject(:recommended_resources) { ResourceRecommendation.new(moment).resources } |
| end | ||
|
|
||
| end | ||
| end |
Description
Phase 1 complete of adding Resource recommendations to Moments show page based on all Moment attributes, matching with Resource tags.
Corresponding Issue
[ifmeorg#1713]
Screenshots
Reviewing this pull request? Check out our Code Review Practices guide if you haven't already!