Fixes #476 - Adds missing location details

[anselmbradford]

• Adds keywords, service areas, and wait times fields to location
  details template.
• Adds categories hierarchy styles.
• Adds service_areas template.
• Performs miscellaneous ruby style guide formatting.
• Adds location details specs (keywords, service areas, and wait times).
• Arranges location details spec checks in order they appear on details
  page.
• Adds/edits description section titles for location and service.

[anselmbradford]

@monfresh Ready for review and merge! Thx

[coveralls]

Coverage increased (+0.05%) when pulling 2de827e on 476-adds-missing-details into 59d032c on master.

[anselmbradford]

Screenshots of changes:

[monfresh]

I would either hold off on displaying the keywords field by default, or document what I wrote below until it's clear what the field is supposed to represent per the OpenReferral spec. In SMC, for example, it's used to improve search results, by adding words or phrases that are most likely to be used to search for this service, but would otherwise not be present in any of the other fields that are being searched, like the description and Category fields for example. Some of these phrases might not make sense to display on Ohana Web Search, like the ones in the screenshot you posted. As an aside, if you notice such keywords, please change them in the admin interface 😄

The keywords field in the API could also be used to add common misspellings. For example, if someone deploys an instance of Ohana Web Search and notices that a lot of people type in emegergy housing, you could add that keyword to all services that offer Emergency Housing, thereby returning correct results even when the term is misspelled. When the keywords field is used that way, you would obviously not want to display those keywords on Ohana Web Search.

If the OpenReferral spec defines the keywords field for a purpose that doesn't allow for adding misspelled words or phrases, then a new field could be created on the API for that purpose only, but it might be best to wait until this is defined.

[monfresh]

I'd prefer to stop using this pattern, please. This is one of the areas I'm planning on refactoring, so the existing code can be cleaned up later, but any new partials we add should adhere to a better pattern. The way the component/detail/template is set up, it behaves very much like a method that takes parameters sent from another partial. That kind of logic is best kept out of the view. If there is a need to DRY things up, that logic belongs in a helper method. However, care must be taken to not make things so DRY that they become cracked. Damp is better than cracked and hard to read and understand.

In the details view, each section represents a different field, and it's possible that one might want to style or mark up some fields differently. By using a template, it makes it harder to customize a particular field. The other problem with this approach is that every time the page is rendered, each field goes through the same logic, even though that logic will never apply to certain fields. It would be faster to render the non-array fields (like Audience, Eligibility, Fees, etc.) directly, than have the code go through a conditional first.

In this instance, the best practice would be to have a separate partial for each field, each with its own custom markup, like the languages partial. This way, there's only one place where you need to look to understand what a partial is rendering. With the existing template, there is too much thinking involved. You have to go to the _body partial, then make a note of the local variables that were sent to the _template partial, then keep track of those variables in your head while reading through the multiple conditionals, then think some more about whether or not the field is an Array, assuming you know off the top of your head what kind it is.

Also, any new content we add should make any user-facing text customizable. So, the h1 for these new fields should be added to config/locales/en.yml and referred to in the partial with the t() method. For the Service fields, I think a good name for the key would be service_fields. So, it would be something like this:

en:
  service_fields:
    eligibility: 'Eligibility'
    fees: 'Fees'

In the view, you would use t('service_fields.fees').
Please maintain alphabetical order within en.yml.
Thanks!

[anselmbradford]

I'd prefer to stop using this pattern, please. This is one of the areas I'm planning on refactoring, so the existing code can be cleaned up later, but any new partials we add should adhere to a better pattern. The way the component/detail/template is set up, it behaves very much like a method that takes parameters sent from another partial. That kind of logic is best kept out of the view. If there is a need to DRY things up, that logic belongs in a helper method.

It sounds like a helper method might be the way to go. From the CSS-side it's preferable to have these in a consistent HTML template, with permutations on the template easily identified. If they all have their own named partial it becomes difficult to know which partials need to be updated and which don't if the underlying shared HTML structure needs to change. I'd like to move more toward a Web Component model where UI components are a self-contained collection of HTML/CSS.

In the details view, each section represents a different field, and it's possible that one might want to style or mark up some fields differently.

Right now they each have a unique class name, so can be styled differently through that. The underlying mark up can be separated out for the ones that are different, as is done now, but it's preferable that the same HTML markup be shared.

By using a template, it makes it harder to customize a particular field.

Ideally, the sections would have underlying components defined through a CSS class and a shared HTML markup and if someone wanted to customized this they'd swap in a different component appearance by changing the class and possibly using a different underlying markup.

E.g. right now we have:

<section class="eligibility"><h1>Eligibility:</h1><span>None</span></section>

Instead we should probably add a component class to this, something like:

<section class="summary eligibility"><h1>Eligibility:</h1><span>None</span></section>

Then if someone wanted to customize this, they'd maybe have a different set of component styles they'd apply with:

<section class="summary-compact eligibility"><h1>Eligibility:</h1><span>None</span></section>

... that maybe was the summary but had less padding/margin and fit on one-line or something.

Additionally, they could do customizations to this specific component instance through the .eligibility class.

Anyway, I'm not sure what that looks like on the Rails side, but point is any re-usable UI chunk in the design should be isolated and have a shared HTML structure with other UI chunks that are the same.

[monfresh]

Given that 1) the HTML structure has already been defined and is not likely to change, and 2) the current template is slowing down the view rendering, the simplest solution I would go with today is to have partials for each field, and you can make sure each partial has the correct HTML structure. Then, if it makes sense to refactor into helper methods, that can be explored. The result of refactoring should never be less efficient or less readable than the previous solution.

As an example, take a look at the "Edit location" form in the admin interface. It contains a bunch of fields, of which many share the same HTML structure. I could extract out the various types of similar fields like in this Railscasts, but I personally wouldn't go down that route. I think it's too much added code and complexity for little benefit.

[monfresh]

Having said the above, this scenario is not as complicated as ActiveModel form objects, and it should be possible to create simple helper methods, but it's a little more involved than you might think.

There will need to be at least 2 helper methods: one for array fields, and one for text fields. The problem with your current template is it tries to be a one-size-fits-all solution, and breaks the Tell, Don't Ask principle.

In your template, every field gets asked the same question before it can be rendered: "Are you an array?" For array fields, the answer will always be "Yes", and for text fields, "No". If you already know the answer beforehand, why waste time asking the question? Instead, you should tell the array fields to do one thing, and text fields to do another thing by creating separate methods for them.

Because the methods act on an object, you will need to pass the Service object as a parameter, along with other parameters, like the field name and h1. However, creating a regular helper module with methods that could be used with any object (not just a Service), would be wrong since we'd like to use an object oriented approach.

This means we'll need to create a separate class, called ServicePresenter for example, which will be instantiated in the _body partial like this for each service: service_presenter = ServicePresenter.new(service), then you can call the class method like this: service_presenter.array_field(:service_areas, t('service_fields.service_areas').

[anselmbradford]

I would either hold off on displaying the keywords field by default

I can remove them. This was based on the request in #476. Who created the keywords in there, some random someone trying out the admin interface?

[monfresh]

I was the one who added those keywords last year because this was an actual entry in the SMC DB that we didn't want to show up in search results.

[anselmbradford]

Ahh I see, the way I read it sounded like you were surprised they were there.

[monfresh]

So, I'm doing some refactoring involving renaming organizations to locations and renaming other variables so that the name more closely matches the object, and I came across the template we've been discussing. I just realized that you're using the same template for both services and locations, which is so wrong :)

In the template partial, the variable being used is org, but its meaning changes based on what you passed to it in _body. Sometimes, org in the template refers to a service, and in other cases, it refers to a location (what we are currently calling an organization). This absolutely needs to change, and as I mentioned earlier, the easiest solution would be to have each field in its own partial. Then, we can take a step back and see if there's a need to extract similar fields into presenter methods. Given then we have 2 objects we are dealing with, we will need 2 presenter classes, and if they both share the same HTML, then one can inherit from the other. But I'm not sure there is much benefit to do that.

So, to keep the ball rolling on this PR, I'm going to push some changes and we should be good to merge after that.

[monfresh]

@anselmbradford I just pushed my changes. Let me know when you're done reviewing and I'll fix conflicts and merge.

[anselmbradford]

Looks good. We'll want to add a class to the detail partials that share the same HTML structure at some point, but doesn't have to happen here. Also, I thought we'd remove the keywords? What's your current thinking on that? I don't have a strong preference, I'd added them per #476 with the thought that perhaps they could be disabled/enabled down the road, but I also think the argument for their removal you had written earlier makes sense.

[monfresh]

Sure, I can comment out the keywords render. I'll fix conflicts and merge in a bit.