ETT-1229: Fix undefined error in sidebar #115
Conversation
|
This isn't what we want as the full view link disappears entirely. I need to look a little bit more at why the full-view URL ends up undefined as I think we probably need to make sure that doesn't happen -- even if there are no results there should still be a URL. |
|
OK, after some digging I realize what's going on here -- this branch doesn't set the fullview-only count & url. I'll need to think about how to adjust this. https://github.com/hathitrust/catalog/blob/main/services/Search/Home.php#L214-L229 |
* Always set fullview_url even if there are no results * Default allitems/fullview urls to blank (current page) (shouldn't be necessary but doesn't hurt)
aelkiss
force-pushed
the
ETT-1229-sidebar
branch
from
b359c7a to
9da7886
Compare
|
@moseshll Ready for review. This is set on both dev-1.catalog (directly against main) and on preview (on top of https://github.com/hathitrust/catalog/tree/ETT-636-trixie). It's a bit bigger change but should be safe enough. I will also try adding a test for this case, which should be doable using the sample catalog. |
|
Test added. |
|
Test passes for me locally but is failing in github. I'll need to look at what's going on. |
|
Looks like the new test is failing on mobile which I did not have enabled. I'll see what's going on there. |
|
On mobile the filters are behind a 'Show Search Filters' button. I think I can probably just remove the requirement that the link is visible -- mainly we just care here that the link exists and isn't empty or filled with garbage. |
aelkiss
force-pushed
the
ETT-1229-sidebar
branch
from
bd5ec9c to
a18d771
Compare
|
I ended up just skipping the test for mobile, for the same reason as we have in the facets spec. |
|
I skipped mobile for the |
moseshll
left a comment
moseshll
left a comment
There was a problem hiding this comment.
I had hoped to A/B test and observe this locally but pear.php.net seems to be acting up so I can't run it. So just going off the code changes and the previews I have no issues.
I'm having trouble replicating the issue in dev, but I think this should fix it. I'm going to cherry-pick this commit on preview so we can see if it fixes it there.
The URL with the error is:
https://preview.catalog.hathitrust.org/Search/Home?lookfor=98731&searchtype=isn&ft=ft&setft=true