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

Canonical collections #84

Merged
merged 3 commits into from
Mar 18, 2015
Merged

Canonical collections #84

merged 3 commits into from
Mar 18, 2015

Conversation

adamnfish
Copy link
Contributor

This PR replaces #80. The canonical container either comes from the FrontJson, or is the first container on the front.

Note that this PR contains two commits, the second specifies the edition network fronts' canonical containers as well. While we wait for the tools to support this feature, the network front canonical containers can be hard-coded. This commit can be rolled back when the tools support is in place and the correct JSON is actually coming through.

Don't merge this yet, it's open for comment/review, I'm going to test it by integrating it with MAPI.

Note that I'm assured there can never be a front with 0 containers. I've raised a an issue (#83) suggesting we capture this by making that a NonEmptyList. That would remove the need for the "no collections" fallback string.

The canonical container either comes from the FrontJson, or is the
first container on the front.
While we wait for the tools to support this feature, the network front
canonical containers can be hard-coded. This can commit can be rolled
back when the toosl support is in place and the JSON is actually
coming through.
@adamnfish
Copy link
Contributor Author

Question: What happens if the editorially chosen canonical collection isn't actually on the front? This shouldn't be possible at the tools level, but is a totally reasonable edge case when caching etc is taken into account.

Should it silently fall back to the top container (which might break things downstream)? Leave the front with no canonical container (which will break things downstream)? Throw an exception?

Maybe it needs to be an Option because of this, but that raises lots more variants of this problem in lots more places.

Thoughts @janua @stephanfowler?

@adamnfish
Copy link
Contributor Author

p.s. This problem is happens at the moment on CODE because the uk, us, au paths don't contain the hard-coded collection IDs, annoyingly. Clearly that's a bug in my hard-coded "waiting for tools" workaround but the question above is relevant to the solution to this bug.

@janua
Copy link
Contributor

janua commented Mar 18, 2015

@adamnfish I think we should fallback, as that is the current behaviour; a reliable fallback consistent in code.
I think we should do both. Make it Option but also have a method of getting the solid underlying type without the Option.

Also extends the hard coded network front fix to the CODE
environment. This part of the code can be removed when the tools
support is added.
@janua
Copy link
Contributor

janua commented Mar 18, 2015

👍

@adamnfish adamnfish mentioned this pull request Mar 18, 2015
@janua janua merged commit 4bf1486 into master Mar 18, 2015
@janua janua deleted the canonical-collections branch March 18, 2015 17:47
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.

2 participants