708 return #1083

squidgetx · 2015-01-04T03:49:56Z

Resolves #708 and includes #1071 so be sure to merge that one first

orenyk · 2015-01-05T05:15:20Z

Attempting to visit /reservations gives me the following:

Mysql2::Error: You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near 'AS reservations  WHERE (start_date = '2015-01-05 05:00:00') AND (due_date >= '20' at line 2: SELECT COUNT(*) FROM (SELECT `reservations`.* FROM `reservations`  WHERE (`reservations`.`start_date` BETWEEN '2014-12-29' AND '2015-01-05') UNION
               SELECT `reservations`.* FROM `reservations`  WHERE (`reservations`.`due_date` BETWEEN '2014-12-29' AND '2015-01-05') AS reservations  WHERE (start_date = '2015-01-05 05:00:00') AND (due_date >= '2015-01-05 05:00:00') AND (checked_out IS NULL) AND (checked_in IS NULL) AND (approval_status = 'auto' OR approval_status = 'approved')

And the problem is in app/views/reservations/index.html.erb:6

Is this a MariaDB issue? I'm not familiar with the AS syntax in SQL.

orenyk · 2015-01-05T05:23:08Z

Looks like it might have to do with how you're using the #count method? It appears to be calling it as a column, not as the actual record count. Not 100% sure what's going on.

squidgetx · 2015-01-05T18:53:05Z

fixed by simply adjusting the scope and text to include reservations that begin in the date range instead of ending in it. it's too bad rails doesn't have a UNION operator for active record; it looks like my monkey patch direct-sql wasn't going to fly anyway.

orenyk · 2015-01-07T05:46:23Z

Tested locally, looks pretty good although I might recommend having the total numbers in the filter buttons / headings (e.g. Returned (241/5121)) so that it's clear that it's just a subset. I still have to read through the code, will do that tomorrow.

orenyk · 2015-01-07T15:54:13Z

app/controllers/reservations_controller.rb

-    @default = false
-    # if no filter is defined
-    return unless @reservations_set.nil?
-    @default = true if AppConfig.first.request_text.empty?


What was this?!

Haha, you should do a pull (or depending on how old your branch is, delete and re-pull because I rebased off master)

No, I meant what does the request text have anything to do with it?

Oh wait that's old code. And yes. I hope I didnt break anything but what would the resevation index method have anything to do with request text??

Ha I think it's a major typo from when I was cleaning up the Reservations controller for rubocop (see 05212fa). I'm not sure where it came from!

orenyk · 2015-01-07T16:20:34Z

Alright, the code looks good. I'd still recommend updating the filter button text to include the total counts, otherwise nice job!

squidgetx · 2015-01-07T16:35:42Z

The only issue with that is that (at least on my 1366px monitor) it doesn't really fit lol. The total counts are reflected by the date selector, which was my compromise; I also think it's a little confusing. Thoughts?

squidgetx · 2015-01-07T18:27:22Z

wow the build passed!

latest changes include some more refactoring since I realized the entire @default variable was useless as well as the addition of a 'view all' option if the admin doesn't want to filter dates

orenyk · 2015-01-07T19:38:29Z

What about a multiline label? Something like

Upcoming
(12/152)

or something along those lines? I know it's a little clunky but otherwise the counts are really confusing, I think.

orenyk · 2015-01-07T20:17:19Z

The total count isn't correct anymore, it's just saying 17 of 17 reservations begin between the specified dates, etc.

orenyk · 2015-01-07T20:18:35Z

app/views/reservations/index.html.erb

+<div id="date_range" class="row">
+  <div class="span2">
+    <h4>Filter By Date:</h4>
+    <p><%= filter_message(@reservations_set, @filter, @view_all) %></p>


should you be using @reservations_source instead?

squidgetx · 2015-01-07T22:40:53Z

Ok, I'll fix that in a sec.

What if we just remove the counts completely from the buttons? @mnquintana?

orenyk · 2015-01-08T03:10:37Z

Hmmm... we could remove the counts from the buttons, but maybe we could have them show up on mouseover? I guess they're not critical, but it would be nice to have them somewhere.

mnquintana · 2015-01-08T12:54:09Z

Hmmm those really shouldn't be buttons at all – they should be tabs or pills: http://getbootstrap.com/2.3.2/components.html#navs

(The benefit of pills is that they can be vertically stacked, which might be necessary to make them fit)

EDIT: Looking at it more I feel like vertically stacked pills/tabs in the left-hand column under the "Filter By Date" box might be the way to go

mnquintana · 2015-01-08T12:58:15Z

app/views/reservations/index.html.erb

 <%= render :partial => "reservations_list", locals: {reservations_set: @reservations_set} %>
+  </div>
+</div>
+


One too many newlines here

mnquintana · 2015-01-08T13:12:32Z

or

(I think the second one looks better)

orenyk · 2015-01-08T13:30:04Z

That makes so much more sense! Agreed about the second one, navigation skills are a higher priority than date selection. @squidgetx, how does it look to you?

squidgetx · 2015-01-08T14:22:58Z

Finished!

orenyk · 2015-01-08T17:31:16Z

I'll quickly review later and will let you know when we can squash / merge :-)

mnquintana · 2015-01-08T19:01:30Z

One last thing - @squidgetx can you make the parentheticals (e.g. (100/6789)) Bootstrap badges?

mnquintana · 2015-01-08T22:59:22Z

I think visually it would work better if you made the active tab badge .badge-info and the others just .badge – blue is more of a highlight color than gray IMO, so it would emphasize that tab being "active".

mnquintana · 2015-01-08T23:00:23Z

Also, the spacing is a little tight – I think you could safely make that a .span3 column.

(It won't look as good, but it's very possible that the badge could overlap "Checked Out" if any of the numbers are big enough – mine almost are)

mnquintana · 2015-01-09T00:48:01Z

app/views/reservations/_date_filter.html.erb

+  <%= form_tag update_index_dates_path, method: :put do %>
+    <%= hidden_field_tag "list[filter]", @filter %>
+    <%= label_tag :start_date, 'Begin' %>
+    <%= text_field "list", "start_date", class: "date_start_no_min span2", value: @start_date.strftime('%m/%d/%Y') %>


I think these look a little less awkward in the wider column if you change them to span3

Agreed, even though they'll be long for dates it will still fit with the rest of the column.

mnquintana · 2015-01-09T00:48:37Z

Alright, looks good! Just make that one change and we should be good to squash and merge.

orenyk · 2015-01-09T02:24:39Z

Looks good to me too. The only thing I can think of is if you want to add a few extra tests for any of the methods you wrote? Nominally these would have been written in advance, but it can't hurt to expand our test suite a little :-). Up to you!

mnquintana · 2015-01-09T14:33:19Z

app/assets/stylesheets/reservations/_index.css.scss

@@ -0,0 +1,3 @@
+.count {


This class is redundant – Bootstrap already provides a .pull-right utility class for this! 😃

Ah, I thought there might be something like that

@default

Implement date filtering for reservations#index method as well as general refactoring of the #index method and controller tests for added functionality basic functionality prettify frontend fix test fix rubocop offenses rubocop2 and spec rubocop3 bs typo bs typo simplify scope adjust scope remove confusing comment refactor + rubocop fix spec fix bugs with default views add view all button cop routes and typo remove @default from spec fix filter message implement table and links for eq objects rubocop offenses badges refactor bad smells cosmetic changes rubocop fix span of date boxes tests fix tests remove unnecessary class

squidgetx · 2015-01-09T14:46:23Z

Definitely should add tests for new functionality

708 return

squidgetx force-pushed the 708_return branch from 442d9ba to 5de50ea Compare January 5, 2015 18:49

squidgetx added the 4 - Needs Review label Jan 6, 2015

squidgetx force-pushed the 708_return branch from 5193927 to f491629 Compare January 7, 2015 15:52

orenyk reviewed Jan 7, 2015
View reviewed changes

mnquintana reviewed Jan 8, 2015
View reviewed changes

mnquintana mentioned this pull request Jan 8, 2015

Upgrade Bootstrap #245

Closed

squidgetx force-pushed the 708_return branch from 6f1ee00 to f4ffd25 Compare January 8, 2015 14:22

mnquintana reviewed Jan 9, 2015
View reviewed changes

orenyk added 3 - ready to merge and removed 4 - Needs Review labels Jan 9, 2015

orenyk mentioned this pull request Jan 9, 2015

Reports #1087

Merged

mnquintana reviewed Jan 9, 2015
View reviewed changes

squidgetx force-pushed the 708_return branch from d2c26af to a2d955f Compare January 9, 2015 14:42

squidgetx force-pushed the 708_return branch from a2d955f to ba5d02c Compare January 9, 2015 14:45

squidgetx added a commit that referenced this pull request Jan 9, 2015

Merge pull request #1083 from YaleSTC/708_return

c732aac

708 return

squidgetx merged commit c732aac into master Jan 9, 2015

orenyk removed the 3 - ready to merge label Jan 9, 2015

mnquintana deleted the 708_return branch February 20, 2015 13:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

708 return #1083

708 return #1083

squidgetx commented Jan 4, 2015

orenyk commented Jan 5, 2015

orenyk commented Jan 5, 2015

squidgetx commented Jan 5, 2015

orenyk commented Jan 7, 2015

orenyk Jan 7, 2015

squidgetx Jan 7, 2015

orenyk Jan 7, 2015

squidgetx Jan 7, 2015

orenyk Jan 7, 2015

orenyk commented Jan 7, 2015

squidgetx commented Jan 7, 2015

squidgetx commented Jan 7, 2015

orenyk commented Jan 7, 2015

orenyk commented Jan 7, 2015

orenyk Jan 7, 2015

squidgetx commented Jan 7, 2015

orenyk commented Jan 8, 2015

mnquintana commented Jan 8, 2015

mnquintana Jan 8, 2015

mnquintana commented Jan 8, 2015

orenyk commented Jan 8, 2015

squidgetx commented Jan 8, 2015

orenyk commented Jan 8, 2015

mnquintana commented Jan 8, 2015

mnquintana commented Jan 8, 2015

mnquintana commented Jan 8, 2015

mnquintana Jan 9, 2015

orenyk Jan 9, 2015

mnquintana commented Jan 9, 2015

orenyk commented Jan 9, 2015

mnquintana Jan 9, 2015

squidgetx Jan 9, 2015

squidgetx commented Jan 9, 2015

708 return #1083

708 return #1083

Conversation

squidgetx commented Jan 4, 2015

orenyk commented Jan 5, 2015

orenyk commented Jan 5, 2015

squidgetx commented Jan 5, 2015

orenyk commented Jan 7, 2015

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

orenyk commented Jan 7, 2015

squidgetx commented Jan 7, 2015

squidgetx commented Jan 7, 2015

orenyk commented Jan 7, 2015

orenyk commented Jan 7, 2015

Choose a reason for hiding this comment

squidgetx commented Jan 7, 2015

orenyk commented Jan 8, 2015

mnquintana commented Jan 8, 2015

Choose a reason for hiding this comment

mnquintana commented Jan 8, 2015

orenyk commented Jan 8, 2015

squidgetx commented Jan 8, 2015

orenyk commented Jan 8, 2015

mnquintana commented Jan 8, 2015

mnquintana commented Jan 8, 2015

mnquintana commented Jan 8, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mnquintana commented Jan 9, 2015

orenyk commented Jan 9, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

squidgetx commented Jan 9, 2015