Spotlight content models #258
Conversation
Pull Request Test Coverage Report for Build 394379217
💛 - Coveralls |
|
ping |
There was a problem hiding this comment.
I think this looks great, @macfarlandian! I only have two minor comments.
| import retrieveContent from "./retrieveContent"; | ||
| import US_ND from "./sources/us_nd"; | ||
|
|
||
| test("returns content for the specified tenant", () => { |
There was a problem hiding this comment.
Will this test fail if the US_ND content is "malformed," e.g. references a key that doesn't map to an actual metric id? If not, I'd love to have some basic unit test that loops through tenants and ensures nothing is malformed.
There was a problem hiding this comment.
this test will not fail in that case. However, you will get a compile-time error on the malformed content object because Typescript will reject any key in that object that isn't part of the MetricTypeId union type.
If the data were coming from an external source then that would be a different story, but since it isn't, I thought it would be reasonable to just rely on the static analysis for this?
There was a problem hiding this comment.
I buy that, if the compile-time error will be caught in the CI we've set up for this project? If it's possible to get a PR merged here without having passed through that check, then I'd request a unit test be created so we don't have bug-fix PRs for accidental misconfigurations. But if it's impossible to get to a merged PR with a misconfiguration, then we're good!
There was a problem hiding this comment.
@jessex see latest commit that failed (although I am a bit peeved that our pre-commit hooks are still not working; it shouldn't have even been possible for me to commit that change!)
There was a problem hiding this comment.
Sold! Thanks for proving that, @macfarlandian
This reverts commit d757539.
|
The checks are kind of a mess on this PR due to a combination of the earlier Github CI outage and my experimentation with an alternative workflow configuration in a different branch, which seems to have confused Github's CI elves. All the checks are now passing; the failures are out-of-date duplicates that won't seem to go away for reasons I do not fully understand. I've had to un-require "Test Public Dashboard" to allow this merge to proceed, I will re-require it immediately after. |
Description of the change
Expanding on some of the patterns prototyped in #257, this establishes the backbone of the new Spotlight data models. There are two main parts to it:
1. Content "management" (
src/contentApi)I use the term loosely because there is barely any management involved: we hard-code all the content for a tenant (North Dakota, the only one we currently have) into a giant JS object and then use a function to look it up by name. There's not much to this other than a big data file and some type definitions. (Some of the copy may wind up being rewritten to suit the new context, but for now it's a direct port from the old site.)
The most notable thing about it is probably the fact that all of the metrics and collections are optional; North Dakota happens to have everything it is currently possible to have, but that is not a guarantee we want to make for any given tenant as we start to add more.
2. Data models (
src/contentModels)These are JS classes that can be instantiated with the relevant bits of the big content tree. Each has its own factory function as well; the one for
Metricisn't really doing anything interesting yet, but as seen in the POC, it will be soon.The
Collectionclass as implemented here is not as strongly typed as the metrics were in that POC. While it is the case that a specific "kind" of Collection should only be related to a set of specific metrics, and that we could describe these relationships in a fully static way for the TS compiler to validate, I'm not actually sure that's worthwhile at this point. Collections are the vaguest part of the content hierarchy as defined in the product specs and I think they would benefit more from flexibility than rigidity at this point. As their functional requirements clarify and evolve, that may change, but in the meantime I don't want to be overly precious with them. (There may be more collection kinds added as we build out this site, but to start with I've only created the ones that parallel the pages on the ND site. They happen to be mutually exclusive but as more are added they are expected to intersect.)Relationships between models are implemented via class embedding. This should make traversing those relationships quite easy, and the model domain is limited enough that I'm not really worried about the circular references getting out of hand. FWIW this is not only well-supported but actively encouraged by MobX, which we'll eventually use to make at least some of these models reactive.
The model relationship graph should mirror the content tree. If a given key is present in the
metricsorcollectionsobject in that tree, a corresponding class instance will be created and mapped to theTenant; if the key is missing, not only will there be no class instance but the key will be absent from the relationship mapping. Given the typing approach discussed above, this is a runtime concern only and there are unit tests to verify it.Type of change
Related issues
Closes #225
Checklists
Development
These boxes should be checked by the submitter prior to merging:
Code review
These boxes should be checked by reviewers prior to merging: