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

Add filters and paging to the workshop materials admin list #16309

Merged
merged 2 commits into from Jul 13, 2017

Conversation

aoby
Copy link
Contributor

@aoby aoby commented Jul 7, 2017

Also consolidated paging logic (now shared between teacher applications and workshop material orders) into a common Pd::PageHelper

Task

image

image

Copy link
Contributor

@mehalshah mehalshah left a comment

Choose a reason for hiding this comment

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

Optional suggestion for making the factory a singleton.

Does the Helper module get exercised as part of a test anywhere?

private

def btn_class(disabled: false)
'btn btn-default' + (disabled ? ' disabled' : '')
Copy link
Contributor

Choose a reason for hiding this comment

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

You could rewrite this as
"btn btn-default#{(disabled && ' disabled').presence}" if you don't like the ternary

view_options(full_width: true)
end

private

def workshop_material_order_params
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if this is standard practice, but should we also have a param helper method here to filter out params other than email / filter and such?

Copy link
Contributor Author

@aoby aoby Jul 12, 2017

Choose a reason for hiding this comment

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

This method is for Strong Parameters since these are being passed into the model in Create. Without specifying them explicitly Rails won't allow them in. This method is called by CanCan's load_and_authorize_resource in the create action and then used to populate the model.

We don't need this for params we reference explicitly in the controller that aren't passed to a model, and I'm not sure what it would add.

end

class PageButtonFactory
def initialize(context, base_params)
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional - you could make the Factory a singleton, and pass in the base params when creating the buttons. Save you the trouble of making a new factory each time. This is a little Java Spring-y - IIRC factories are typically singletons there

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only reason I created the factory was to only need to specify base_params once. If we prefer to supply that for each button, then they can just be private methods in the helper.

Now that I look at it, that's probably simpler. I'll remove the factory and:

button_factory.new_page_button('<<', page: 1, disabled: collection.first_page?)

will become

new_page_button('<<', base_params.merge(page: 1), disabled: collection.first_page?)

@aoby aoby merged commit d0fc7ec into staging Jul 13, 2017
@aoby aoby deleted the pd-order-list-paging branch July 13, 2017 17:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants