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

Sort 2nd level browse pages #182

Merged
merged 2 commits into from Jul 12, 2016

Conversation

Projects
None yet
3 participants
@jackscotti
Contributor

jackscotti commented Jul 7, 2016

content-store does not retain the order of the items that are sent in the links hash, therefore in order to allow custom 2nd level browse pages ordering we have to read the links order from the details hash.

Extra:

  • Added a test to make sure that top-level-browse-pages are always alphabetically ordered
  • Upgraded Slimmer

Depends on: alphagov/collections-publisher#206

paired with @rubenarakelyan

def second_level_browse_pages
links = linked_items("second_level_browse_pages")
if second_level_pages_curated?
if second_level_pages_curated? and ordered_second_level_browse_pages

This comment has been minimized.

@tijmenb

tijmenb Jul 7, 2016

Member

Prefer to use && instead of and (govuk-lint would've told you, but it's not running on this repo).

If you're worried about this being nil (perhaps unnecessarily?) you could also do:

details["ordered_second_level_browse_pages"].to_a.index(link.content_id)

This comment has been minimized.

@jackscotti

jackscotti Jul 11, 2016

Contributor

The worry it's probably unnecessary given that collections-publisher is the only app able to publish mainstream browse pages. Might remove this commit after product review.

@jackscotti jackscotti force-pushed the sort-2nd-level-browse-pages branch from f8bd993 to 2ee881c Jul 11, 2016

@jackscotti jackscotti changed the title from [do not merge]Sort 2nd level browse pages to Sort 2nd level browse pages Jul 11, 2016

}}
end
def add_first_level_browse_pages(second_level_browse_pages, order_type)

This comment has been minimized.

@MatMoore

MatMoore Jul 11, 2016

Contributor

add_first_level_browse_pages takes second_level_browse_pages as input.... 😖

I'm wondering if there's a better name to use for this, or maybe you could make second_level_browse_pages a named argument so it's obvious this is actually second level?

This comment has been minimized.

@jackscotti

jackscotti Jul 11, 2016

Contributor

I understand the pain in reading this. What if I rename the signature to:

def add_first_level_browse_pages(links, order_type)

This comment has been minimized.

@MatMoore

MatMoore Jul 11, 2016

Contributor

If links is the full links hash then I think this is a lot better 👍

Or def add_first_level_browse_pages(links:, order_type:)

},
details: {
second_level_ordering: order_type,
ordered_second_level_browse_pages: second_level_browse_pages.map { |page| page[:content_id] }

This comment has been minimized.

@MatMoore

MatMoore Jul 11, 2016

Contributor

Can ordered_second_level_browse_page ever be a subset of the the full list instead of the full thing? If so we should have a test for it. I would expect non curated pages to be ignored.

This comment has been minimized.

@jackscotti

jackscotti Jul 11, 2016

Contributor

Not, it will never be a subset. Full list.

This comment has been minimized.

@MatMoore

MatMoore Jul 11, 2016

Contributor

OK, I'm happy as long as collections publisher enforces this.

Permit curated 2nd level browse pages
content-store does not retain the order of the items that are sent in the links
hash, therefore in order to allow custom 2nd level browse pages ordering we
have to read the links order from the details hash.

Extra: Added a test to make sure that top-level-browse-pages are always
alphabetically ordered

@jackscotti jackscotti force-pushed the sort-2nd-level-browse-pages branch from 02ef277 to b957aeb Jul 12, 2016

@MatMoore MatMoore merged commit 141f77b into master Jul 12, 2016

1 check passed

default Build #616 succeeded on Jenkins
Details

@MatMoore MatMoore deleted the sort-2nd-level-browse-pages branch Jul 12, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment