Allow talk proposals to elect a particular month. Closes #33 #44
Conversation
|
Now that I've sent this PR, I'm actually reconsidering one aspect of it. In it's current form the following scenario is possible: |
| validates :proposed_month, allow_blank: true, numericality: { | ||
| greater_than: 0, | ||
| less_than_or_equal_to: 12 | ||
| } |
There was a problem hiding this comment.
You could do this more concisely as something like:
validates_inclusion_of :proposed_month, :in => 1..12
|
In the abscence of #30 and #31 being completed, I think the simplest way to do this would be to have a helper method that would generate the dates of the next 12 meetups or so and jam those into the select tag. This would also allow the display of what day that was, rather than just the month. This could be more work than its worth, however, so I'm not completely opposed to your implementation. WDYT? |
|
Yep that's the route I referred to in my comment above. The above line issues you pointed out are moot with the new solution. Updated PR incoming shortly. |
|
Alright @phiggins have a gander. |
|
Overall this looks good @benlakey. 👍 Please let me know if the comments I made don't make sense or if you'd like to discuss. |
| validate :proposed_date_valid? | ||
|
|
||
| def proposed_date_valid? | ||
| return if proposed_date.blank? |
There was a problem hiding this comment.
This should probably return true because the name implies it.
There was a problem hiding this comment.
On second thought, I was thinking of the case where this used as an external query method, but that's not a use case we have. Make this private instead.
There was a problem hiding this comment.
I'll do one better: I'll rename it "validate_proposed_date" and make it private.
|
You should add a test for submitting a talk with a valid date and verifying that the record has the correct date associated with it. |
| 1.upto(12).map do |increment| | ||
| future_month = Date.today >> increment | ||
| option = Date.new(future_month.year, future_month.month, 1) | ||
| option += 1 until option.tuesday? |
There was a problem hiding this comment.
Next month's meeting proves this code wrong. It is too exacting and doesn't take reality into account.
12 months out seems completely unrealistic and prone to failure/disappointment.
There was a problem hiding this comment.
@zenspider On your first statement I'd argue that no pre-fab select list will be able to predict the future. When #33 is complete the data can be fed from there. So for now this is the best we can do.
As far as the 12-months-out case; Would 6 months be preferable? I have no strong opinions about this so if you want to provide a value I'll plug it in.
There was a problem hiding this comment.
@phiggins I'm not sure what you're looking for with the test you suggested adding. The proposed_date attribute has no special logic around it so I don't know what value we'd get by testing that a thing got saved via activerecord and pulling it out again (seems like that'd just be testing activerecord). The validation aspect already has a test. Let me know if you had something different in mind.
There was a problem hiding this comment.
I meant that if there was a test for submitting a talk through the controller that you should augment it to include a proposed date. This would help verify that the date survives going through the controller and strong_params / mass assignment, etc.
Again, not a huge deal, but a nice to have.
|
Oops I didn't mean to close and re-open this... |
|
Ok @phiggins I incorporated the last of your feedback. This should be pullable now unless @zenspider has strong feelings about the short-term solution we've got here (pre-fab select list until we have the calendar solution in place). |
|
Closing in favor of future calendar solution |
No description provided.