Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds filter clearing logic to button on order cycle admin page #2479

Conversation

adam-conway
Copy link

What? Why?

Closes #2362

On the admin order cycles page (/admin/order_cycles) there is a 'Clear All' button that doesn't currently do anything. This PR adds a method in the angular file to clear all of the filters when this button is clicked.

What should we test?

We should test that it does in fact clear all the filters if they've been set and all available order cycles show in the list.

Release notes

Fixes the clear all button on the admin order cycles page
Changelog Category: Fixed

Copy link
Contributor

@sauloperez sauloperez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for your contribution @adam-conway !

@@ -6,6 +6,13 @@ angular.module("admin.orderCycles").controller "OrderCyclesCtrl", ($scope, $q, C
$scope.involvingFilter = 0
$scope.scheduleFilter = 0

$scope.resetSelectFilters = ->
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@luisramos0 luisramos0 self-requested a review July 18, 2018 15:20
@@ -6,6 +6,13 @@ angular.module("admin.orderCycles").controller "OrderCyclesCtrl", ($scope, $q, C
$scope.involvingFilter = 0
$scope.scheduleFilter = 0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Adam, welcome! These two lines above are redundant as they are inside resetSelectFilters which is called just below, isn't it?

@myriamboure
Copy link
Contributor

Hey @adam-conway ! Great to see your first PR :-) It seems you are pretty close to get it approved, do you think you can take into account the change requested so that we can move it forward into the pipe? Thanks a lot!

page.should have_selector "#listing_order_cycles tr.order-cycle-#{oc1.id}"
page.should have_selector "#listing_order_cycles tr.order-cycle-#{oc2.id}"
page.should have_selector "#listing_order_cycles tr.order-cycle-#{oc3.id}"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we take advantage of working on this to change to RSpec's new syntax (expect(page).to have(stuff)) ?

@daniellemoorhead
Copy link
Contributor

Hi @adam-conway, do you think you'll get time to make the requested changes so we can test and merge soon? We try to keep PRs in play to as small a number as possible, and this has been open for 3 weeks without change...

@daniellemoorhead
Copy link
Contributor

Closing this PR due to inactivity (>1 month), and moving the issue back to dev ready.

@adam-conway if you have time in the future feel free to reopen and finish it off 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants