Resolves #55: rpt by site#112
Conversation
|
Thanks for working on this! It looks like Travis is failing on the style check. Most of the issues that it reported are auto-correctable, so a |
leesharma
left a comment
leesharma
left a comment
There was a problem hiding this comment.
Hooray, nice PR! 🎉 It'll be really nice to have this feature in the app.
It looks like the build is passing now, but I added a few comments and suggestions. Let me know if you have any questions or disagree with anything!
| <img src="<%= event.signature %>" | ||
| style="width: 15em; | ||
| padding: 2px; | ||
| border: 3px solid #069;" /> </td> |
There was a problem hiding this comment.
Nice! Can this be turned into a CSS class?
| .order(:occurred_at) | ||
| end | ||
|
|
||
| def pull_join(site_id = 0) |
There was a problem hiding this comment.
Our database is zero-indexed, so site_id = 0 might be a valid site. I'd set the fallback to -1 or something similar so that all valid sites are reachable.
|
|
||
| class SignaturesReportsController < ApplicationController | ||
| before_action :authenticate_user! | ||
| expose(:work_sites) do |
There was a problem hiding this comment.
Manipulating arrays of tuples isn't a common practice in ruby (code tends to be very object oriented and method-focused), so I worry that having this in the main body of the controller will be confusing down the line. I know that the select box takes this format for its input, but can we isolate this so that it doesn't get tangled up in the the rest of the application logic?
Since it's basically a scope on WorkSite, maybe something like this?
class WorkSite
# ...
def select_box_input
fallback = ['All Sites', -1] # see justification for -1 in another comment
addresses = where(active: true).order(:address).pluck(:address, :id) # like your #map, but at the db level
addresses.unshift fallback
end
endIt's not a perfect solution (there's still some array manipulation going on later in the controller), but this provides a clear place for the tuple-related code to live and discourages mingling.
|
Lee, thank you very much for the feedback. I've made the changes you recommended, but I must point out that using zero as a legitimate database id violates database best practices. Ids are expected (by database geeks like me at least) to be positive integers. |
3 participants
After many delays (travel, death in family, holidays) I'm finally done with this. I rearranged the input fields and added a header to indicate which site (or "All Sites") the report is for. Tests look for this. Please take a look; thanks.