Autocomplete for mentors and participants #209

aayush19saxena · 2017-02-25T20:56:58Z

Hi guys @mattharris5 @mitchellharper12 ,

This is an improved version of searching. I used Josue's last pull request and did most of the computational work in the person.rb.

It's been a while pushing something, I'll start working towards DreamSIS more once as the spring quarter gets closer!

mitchellharper12

This whole new change review system in Github is nice, letting you put all the comments together at once.

Overall, the underlying purpose of this code is pretty good, I'm just not sure if there are any more performant options for performing these types of searches. Also, the cases where multiple strings being passed need approval from @mattharris5 to whether the behavior is correct.

As a last note, we aren't in a good habit of committing test code with each feature PR, probably because we don't have a good unit test framework. That's one of my goals to set up, and in the future I think it would make sense to require tests with newly merged features and make a habit of it.

mitchellharper12 · 2017-03-02T06:41:49Z

app/views/layouts/application.html.erb

@@ -136,7 +136,7 @@
 			<div class="git branch">Customer <strong><%= Customer.name_label rescue nil %></strong></div>
 			<div class="git branch">Tenant <strong><%= Apartment::Tenant.current_tenant rescue nil %></strong></div>
 			<div id="dev-inspector" style="display:none">
-				<div class="session-info"><pre><%= session.to_yaml %></pre></div>
+				<div class="session-info"><pre><%#= session.to_yaml %></pre></div>


Is there any reason this is commented out now?

mitchellharper12 · 2017-03-02T06:49:16Z

app/models/people/person.rb

@@ -131,6 +131,51 @@ def self.object_filters
    []
  end

+  def self.find_by_name(name) 


This method name is confusing. Generally, if there's a method on a model named find_by_..., I expect the method will return an instance of that model. For example, if the method was named find_by_id, I would expect the method would take an integer and return the object that has the associated ID.

However, this method does not return Person objects. It returns an array of 4 strings.

I would rename this to make it more clear that the method is not doing any "finding", but is instead parsing a name and returning some relevant data. I would also add a comment above the method stating what the method takes and what it returns, because it is not immediately clear on its own.

mitchellharper12 · 2017-03-02T06:50:18Z

app/models/people/person.rb

+    name = name.downcase
+    args = name.split(" ")
+    conditions = generate_conditions(args, name)
+    return conditions


These two lines above can be combined, no need for the assignment to conditions.

mitchellharper12 · 2017-03-02T06:58:33Z

app/models/people/person.rb

+    first = ""
+    middle = ""
+    last = ""
+    args = args.each { |s|  s.strip! }


This line ends up doing the right thing, but not in the way you expect...

The assignment is actually redundant. Because you are using the in-place strip! method, each string in the args array will already be stripped. When you call .each, the original array is returned, so the original array is being set to itself here. It just so happens that by calling the in-place search, you are stripping each string directly in the args array.

So if this line were changed to only read args.each { |s| s.strip! }, the behavior would be the same. The more standard way of writing this would be to say

args = args.collect { |s| s.strip }

which does not use an in-place strip. Additionally, as a Ruby trick, whenever you have a block that takes one argument and calls a method on that one argument, you can rewrite the method to take a Proc instead:

args = args.collect(&:strip)

mitchellharper12 · 2017-03-02T07:03:21Z

app/models/people/person.rb

+      end
+      conditions = ["(LOWER(firstname) LIKE :firstname AND LOWER(lastname) LIKE :lastname AND LOWER(middlename) LIKE :middlename)", first, middle, last]
+    end
+    return conditions


It may make more sense to return an object rather than an array, that way you don't have to remember what each array index represents. For example, this could be what's returned:

{ firstname: "Keisha", middlename: "Test", lastname: "Test", sql_where: "SQL LIKE(STUFF) WHERE STUFF ETC" }

mitchellharper12 · 2017-03-02T07:08:06Z

app/models/people/person.rb

+        first = args[0]
+        middle = args[1]
+      end
+      conditions = ["(LOWER(firstname) LIKE :firstname AND LOWER(lastname) LIKE :lastname AND LOWER(middlename) LIKE :middlename)", first, middle, last]


Of all of the SQL statements, this one seems like the most likely to exhibit incorrect, or at least imperfect, behavior.

Sometimes middlenames are stored with first names for UW mentors because that's how data is returned from the student database. Also, sometimes people have two-word first or last names. I'm also not sure the best way to handle this case, as by doing straight SQL like this there are a number of edge cases depending on how you structure the query.

mitchellharper12 · 2017-03-02T07:09:22Z

app/controllers/participants_controller.rb

+      first = generated_conditions[1]
+      middle = generated_conditions[2]
+      last = generated_conditions[3]
+
      conditions << "high_school_id = :high_school_id" if params[:high_school_id]
      conditions << "grad_year = :grad_year" if params[:grad_year]


Do these two lines above work together? There are no spaces at the start or end of lines, so I'm wondering if there's a chance that the string won't have spaces where it needs them.

mitchellharper12 · 2017-03-02T07:10:05Z

app/controllers/participants_controller.rb

+                                        :conditions => [conditions, 
+                                                        {:firstname => "%#{first}",
+                                                        :lastname => "%#{last}",
+                                                        :middlename => "%#{middle}",


Do we also need wildcard characters after the names? Here, they are only before, which may not be the behavior we want.

mitchellharper12 · 2017-03-02T07:11:34Z

Gemfile.lock

@@ -1,7 +1,8 @@
 GEM
  remote: https://rubygems.org/
  specs:
-    RedCloth (4.2.9)
+    CFPropertyList (2.3.5)


I don't think we have strict guidelines as to whether or not you should commit your Gemfile.lock with feature PRs, but to me it makes more sense to add it in a separate commit instead. Not a huge deal either way though.

mitchellharper12 · 2017-03-02T07:14:40Z

app/models/people/person.rb

+        last = args[1]
+        middle = args[1]
+      end
+      conditions = ["(LOWER(firstname) LIKE :firstname AND(LOWER(lastname) LIKE :lastname OR LOWER(middlename) LIKE :middlename))", first, middle, last]


This could also be the case where a person has two letter first name or last name. It's just a decision whether or not we want to gracefully handle this case.

aayush19saxena added 2 commits February 25, 2017 11:42

Setting up environment

cd49ef6

Finishing autocomplete for mentors and participants

e2280a7

mitchellharper12 reviewed Mar 2, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Autocomplete for mentors and participants #209

Autocomplete for mentors and participants #209

aayush19saxena commented Feb 25, 2017 •

edited

Loading

mitchellharper12 left a comment

mitchellharper12 Mar 2, 2017

mitchellharper12 Mar 2, 2017

mitchellharper12 Mar 2, 2017

mitchellharper12 Mar 2, 2017

mitchellharper12 Mar 2, 2017

mitchellharper12 Mar 2, 2017

mitchellharper12 Mar 2, 2017

mitchellharper12 Mar 2, 2017

mitchellharper12 Mar 2, 2017

mitchellharper12 Mar 2, 2017

Autocomplete for mentors and participants #209

Are you sure you want to change the base?

Autocomplete for mentors and participants #209

Conversation

aayush19saxena commented Feb 25, 2017 • edited Loading

mitchellharper12 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aayush19saxena commented Feb 25, 2017 •

edited

Loading