BZ-9424 SerSol 360 Handles Duplicate ISSNs #140
Conversation
|
Hey @Evroble , I think the commit I pushed should get tests passing. I think browzine-discovery-service-adapters/src/360-core/browzine-360-core-adapter.js Lines 100 to 114 in 55e3b24 Let me know if you agree - thanks! |
There was a problem hiding this comment.
Overall LGTM @Evroble , but we'll need to delete some of these console logs, and probably one of the comments, as long as the test change I made looks good.
Nice job on the sleuthing and getting to the bottom of this mystery! Once those console logs are gone, this is good to merge.
| @@ -351,23 +351,6 @@ describe("SerSol 360 Core Model >", function() { | |||
| }); | |||
| }); | |||
|
|
|||
| describe("serSol360Core model getEndpoint method >", function() { | |||
There was a problem hiding this comment.
Thanks for axing the duplicate 👍
| /* NOTE to Karl: | ||
| Essentially what i've tried to do is fully seperate this test from the others, but the code seems to refuse to recognize the data that i've set up | ||
| I've added logs so you can run locally and see what i'm seeing, but it seems like it's not recognizing the data | ||
| Previously I tried using 'searchResults' instead of 'searchResultsSameIssn' but then I was getting data from the core test again | ||
| */ |
There was a problem hiding this comment.
If my changes look good we should be able to remove this comment.
| if (index >= elements.length) { | ||
| var target = undefined; | ||
| } else { | ||
| var target = elements[index]; | ||
| } |
There was a problem hiding this comment.
I think this should work - if you wanted to keep the ternary approach, you could also just do:
var target = element.length > index ? element[index] : undefined;
Since index is 0-based, and length is 1 based, then it should work fine.
| console.log(titles.length, 'how many titles do we have?'); | ||
| console.log(titles, 'our titles'); |
| title.endpoint = getEndpoint(issn); | ||
| title.shouldEnhance = shouldEnhance(issn); | ||
|
|
||
| if (title.shouldEnhance) { | ||
| titlesToEnhance.push(title); | ||
| } | ||
| console.log(titles, 'our list of titles'); |
|
|
||
| var target = element.length > 0 ? element[0] : undefined; | ||
| function getTarget(index) { | ||
| console.log(index, 'our index inside getTarget'); |
|
@karlbecker - so I believe i've figured out why my tests won't pass. I left a note in the code but basically in our tests we mock the response through the call On a separate note, I'd also like us to sit down and maybe add in comments/notes about how the setup of these tests are working, because it is very confusing. |
|
Works for me @Evroble ! |
Summary - BZ-9424
A customer reported seeing duplicate links on one entry when searching for a specific journal. We had seen a similar issue before in https://thirdiron.atlassian.net/browse/BZ-5276 and thought we had solved it. This time, the issue has stemmed from duplicate issns being listed, something we did not account for previously. This change adjusts the code to account for duplicate issns and ensure our links are not placed on the same listing multiple times.
Description
BEFORE:
AFTER:
Implementation Notes
The change largely involves two functions getIssn and getTarget
1). getIssn - this change involves targeting the first issn listed rather than the last, as it seems to be the one more often referenced/used.
2). getTarget - this change involves taking the index from
titles.forEachand using it to pair the appropriate title with the appropriate element so titles[0] attaches to elements[0].Deploy Prerequisites
Deploy Precautions
Deploy Informational Notes