[1643] Hide Unavailable Equipment Items - #1668
Conversation
9ff6aa1 to
11439c2
Compare
| <%= paginate @page_eq_models_by_category, | ||
| params: { controller: 'catalog', action: 'index' } %> | ||
| <%= form_tag update_user_per_cat_page_path, | ||
| <%= form_tag update_user_per_cat_page_path, |
There was a problem hiding this comment.
I'm aware my IDE did something strange to the whitespace - will follow up
There was a problem hiding this comment.
I think it just converted the spaces to tabs to match the rest of the file. To be honest, I think it's fine.
|
Ready for review! |
AlanLiu96
left a comment
AlanLiu96
left a comment
There was a problem hiding this comment.
Overall amazing job. :) Really clear tests and efficient js. I left a few comments below, but they're mainly small changes.
| } else { | ||
| $("#toggle_unavailable").text("Hide unavailable"); | ||
| } | ||
| $(".col-md-4").each(function() { |
There was a problem hiding this comment.
While this works, could we use class/div tags and attach to those instead of the (".col-md-4") and (".col-md-3") css tags? (Ex. in the future we might make something else width 4, and wonder why it suddenly keeps disappearing) :)
| page.reset! | ||
| end | ||
|
|
||
| it 'shows all items by default' do |
There was a problem hiding this comment.
awesome tests, really clear and easy to read
| end | ||
|
|
||
| it 'reshows all items when clicked again' do | ||
| click_button "toggle_unavailable" |
There was a problem hiding this comment.
Do the last two tests depend on them executing in this order? I'm not sure if it's guaranteed that the tests won't be swapped.
|
Also, I think style check is causing the build to fail -- |
9037e95 to
1bc2785
Compare
6966e9b to
91aedd4
Compare
esoterik
left a comment
esoterik
left a comment
There was a problem hiding this comment.
some comments on your test
| empty_cart | ||
| Capybara.ignore_hidden_elements = false | ||
| @eq_model1 = FactoryGirl.create(:equipment_model, category: @category) | ||
| FactoryGirl.create(:equipment_item, equipment_model: @eq_model1) |
There was a problem hiding this comment.
I think we have an equipment_model_with_item factory
| context 'when hide unavailable button is clicked' do | ||
| before(:each) do | ||
| empty_cart | ||
| Capybara.ignore_hidden_elements = false |
| Capybara.ignore_hidden_elements = false | ||
| @eq_model1 = FactoryGirl.create(:equipment_model, category: @category) | ||
| FactoryGirl.create(:equipment_item, equipment_model: @eq_model1) | ||
| @eq_model1[:overdue_count] = 1 |
There was a problem hiding this comment.
you should actually create an overdue reservation for this because it's a feature spec
| @eq_model1[:overdue_count] = 1 | ||
| @eq_model1.save! | ||
| @eq_model1.reload | ||
| page.reset! |
There was a problem hiding this comment.
why is this necessary? you haven't visited a page yet
| @eq_model1.save! | ||
| @eq_model1.reload | ||
| page.reset! | ||
| visit root_path |
There was a problem hiding this comment.
this belongs in the individual it blocks
|
|
||
| after(:each) do | ||
| Capybara.ignore_hidden_elements = true | ||
| @eq_model1.destroy |
| after(:each) do | ||
| Capybara.ignore_hidden_elements = true | ||
| @eq_model1.destroy | ||
| page.reset! |
There was a problem hiding this comment.
this also shouldn't be necessary
|
|
||
| it 'shows all items by default' do | ||
| expect(page).to have_css('.availability-num', text: 1, visible: true) | ||
| expect(page).to have_css('.availability-num', text: 0, visible: true) |
4 participants
Resolves #1643