-
Notifications
You must be signed in to change notification settings - Fork 0
Refactor how TIMDEX records are nomalized #242
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
Conversation
Pull Request Test Coverage Report for Build 18603748433Details
💛 - Coveralls |
26d73e3
to
6f0b747
Compare
6f0b747
to
5be6481
Compare
5be6481
to
cf4d71a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a bug in the GDT view that prevents full record views from working and I have a suggestion to change a method name to match the normalized keys. I also think maybe there is a partial that should be deleted but I didn't dig too deeply to see if I'm correct.
I believe the rest is comments/questions/info for later.
Why these changes are being introduced: Prior to USE, TIMDEX records were largely normalized in the view layer. While not ideal, this made some sense given that TIMDEX was the sole source we were working with in the UI. Now that we have introduced Primo results in USE, we should normalize TIMDEX records similarly, such that the two share a similar data structure. Relevant ticket(s): * [USE-73](https://mitlibraries.atlassian.net/browse/USE-73) How this addresses that need: This introduces NormalizeTimdexReuslts and NormalizeTimdexRecord models to parallel the Primo models. The normalization models share a structure to the extent that it is meaningful. Source-specific fields that will be used in the application are indicated as such. Side effects of this change: * Modifications to various parts of the view layer were necessary to retrofit this change. * In some cases, we are only mapping the necessary data. (E.g., TIMDEX links array only includes source link.) This is subject to change as we learn more about UX requirements. * We are still using separate result partials for TIMDEX and Primo, until we have a better sense of how different the requirements are for those sources. * The Primo result partial has been minimized to more closely resemble the TIMDEX partial. This is also subject to change based on UX requirements.
f0cb77c
to
a7d2d68
Compare
Why these changes are being introduced:
Prior to USE, TIMDEX records were largely normalized in the view layer. While not ideal, this made some sense given that TIMDEX was the sole source we were working with in the UI.
Now that we have introduced Primo results in USE, we should normalize TIMDEX records similarly, such that the two share a similar data structure.
Relevant ticket(s):
How this addresses that need:
This introduces NormalizeTimdexReuslts and NormalizeTimdexRecord models to parallel the Primo models. The normalization models share a structure to the extent that it is meaningful. Source-specific fields that will be used in the application are indicated as such.
Side effects of this change:
retrofit this change.
links array only includes source link.) This is subject to change as
we learn more about UX requirements.
the TIMDEX partial. This is also subject to change based on UX
requirements.
Developer
Accessibility
New ENV
Approval beyond code review
Additional context needed to review
I'd like the reviewer's opinion on minimizing the Primo result partial and/or selectively normalizing data (see the note about the links method in the commit message). It's a bit of a chicken-and-egg situation, since I assume we may want more data in the results at some point, but it's not clear yet what.
Code Reviewer
Code
added technical debt.
Documentation
(not just this pull request message).
Testing