Render account AJAX information with server-side templates #4175
Conversation
Requires less JS code and makes it easier to customize the notification badges.
|
Demian asked me to take a look at this PR. I checked the test branch in every theme and compared with dev. It seems to me that the little boxes with numbers work the same and appear the same in test vs. dev except for ONE DIFFERENCE. The difference is that in the test branch, the mouseover text explaining what the color-coded numbers stand for is rendered differently in the bootstrap5 themes. On dev, that popout mouseover text appears as follows in ALL FIVE THEMES (white text on black background, located above the item, with a little pointer arrow as if it were a speech bubble):
On test, it looks the same as above in the legacy themes. But in the bootstrap5 themes, it looks like this (black text on white background, located below the item, no pointer):
My screenshots don't display my pointer hand, but in both cases I was mousing over the 1 (items due later) box. Nitpicky as always -- but I thought you might be interested to know. Hope this is helpful! |
|
@sturkel89 Thanks for testing! I'll get back to fixing this early next year. :) |
|
@sturkel89's testing revealed an issue I didn't really think about: the rendering of badges is cached as done by the theme that was active when the data was initially fetched, and gets stale if theme is switched. I'll need to rethink this a bit further. |
Can we just factor the theme name into the cache key, or is it not that simple? |
That could be done. I just don't really like it that we are doing extra work where we should already have everything we need. Caching the data server-side would allow us to just render the badges when rendering the menu. And if the initial page after login is e.g. checkouts, we could use the same data fetched to show the list of checked out items to show the badges for them instead of triggering another ILS fetch via AJAX. Same with the case where things have changed, and we currently just trigger an update via AJAX. |
|
Thinking about it a bit more, there's a further problem if the user can change the theme; it could easily result in one theme displaying current data while the other still shows stale data. |
|
But I think there's a workaround... Maybe I can get this working. |
|
Ok, the latest commit does theme-specific caching, but adds some complexity I'm not satisfied with. However, I think we need VUFIND-1652 to cache things server-side. I can't do much more at this point, but I'll add a ticket so that I can return to this later. |
There was a problem hiding this comment.
Thanks, @EreMaijala, this looks pretty reasonable to me, though I haven't run tests or done hands-on testing yet. One small question in the meantime; @sturkel89 will give this more attention when we're back in the office starting on Monday!
demiankatz
added
the
architecture
There was a problem hiding this comment.
@EreMaijala, are tests passing for you? In my environment, I'm failing with:
There was 1 error:
1) VuFindTest\Mink\AccountMenuTest::testGlobalCacheClearing
TypeError: ceil(): Argument #1 ($num) must be of type int|float, string given
/usr/local/vufind/module/VuFind/tests/integration-tests/src/VuFindTest/Mink/AccountMenuTest.php:236
--
There were 7 failures:
1) VuFindTest\Mink\AccountMenuTest::testAccountIcon with data set "good" ([[[0, 1, 1]], [[0, 1, 1]], [[0, 1, 1]]], '.account-status-good')
Element not found: #account-icon .account-status-good index 0
Failed asserting that null is of type object.
/usr/local/vufind/module/VuFind/src/VuFindTest/Integration/MinkTestCase.php:528
/usr/local/vufind/module/VuFind/tests/integration-tests/src/VuFindTest/Mink/AccountMenuTest.php:347
2) VuFindTest\Mink\AccountMenuTest::testAccountIcon with data set "warning" ([[[1, 2]]], '.account-status-warning')
Element not found: #account-icon .account-status-warning index 0
Failed asserting that null is of type object.
/usr/local/vufind/module/VuFind/src/VuFindTest/Integration/MinkTestCase.php:528
/usr/local/vufind/module/VuFind/tests/integration-tests/src/VuFindTest/Mink/AccountMenuTest.php:347
3) VuFindTest\Mink\AccountMenuTest::testAccountIcon with data set "danger" ([[[1000000, '$...yikes', 3]], [[1, 3]]], '.account-status-danger')
Element not found: #account-icon .account-status-danger index 0
Failed asserting that null is of type object.
/usr/local/vufind/module/VuFind/src/VuFindTest/Integration/MinkTestCase.php:528
/usr/local/vufind/module/VuFind/tests/integration-tests/src/VuFindTest/Mink/AccountMenuTest.php:347
4) VuFindTest\Mink\AccountMenuTest::testAccountIcon with data set "danger overrides warning" ([[[2, 1, 3]]], '.account-status-danger')
Element not found: #account-icon .account-status-danger index 0
Failed asserting that null is of type object.
/usr/local/vufind/module/VuFind/src/VuFindTest/Integration/MinkTestCase.php:528
/usr/local/vufind/module/VuFind/tests/integration-tests/src/VuFindTest/Mink/AccountMenuTest.php:347
5) VuFindTest\Mink\AccountMenuTest::testAccountIcon with data set "danger overrides good" ([[[1, 3], [1, 1]]], '.account-status-danger')
Element not found: #account-icon .account-status-danger index 0
Failed asserting that null is of type object.
/usr/local/vufind/module/VuFind/src/VuFindTest/Integration/MinkTestCase.php:528
/usr/local/vufind/module/VuFind/tests/integration-tests/src/VuFindTest/Mink/AccountMenuTest.php:347
6) VuFindTest\Mink\AccountMenuTest::testAccountIcon with data set "warning overrides good" ([[[1, 2], [1, 1]]], '.account-status-warning')
Element not found: #account-icon .account-status-warning index 0
Failed asserting that null is of type object.
/usr/local/vufind/module/VuFind/src/VuFindTest/Integration/MinkTestCase.php:528
/usr/local/vufind/module/VuFind/tests/integration-tests/src/VuFindTest/Mink/AccountMenuTest.php:347
7) VuFindTest\Mink\AccountMenuTest::testAccountIcon with data set "good overrides none" ([[[1, 1], [0, 'none', 0]]], '.account-status-good')
Element not found: #account-icon .account-status-good index 0
Failed asserting that null is of type object.
/usr/local/vufind/module/VuFind/src/VuFindTest/Integration/MinkTestCase.php:528
/usr/local/vufind/module/VuFind/tests/integration-tests/src/VuFindTest/Mink/AccountMenuTest.php:347
ERRORS!
Tests: 3114, Assertions: 20873, Errors: 1, Failures: 7.
There was a problem hiding this comment.
Happy New Year! It looks like the issue I identified in December is now fixed.
The popout explanations for each of the numbers displayed in the My Account menus work the same in the bootstrap5 themes as they do in the legacy themes, and test works the same as dev.
Bootstrap5 (with Fines selected but mousing over ILL Requests) :
Sandal5 (with Profile selected but mousing over Storage Retrieval Requests):
Bonus screenshot: In reference to the conversation at the Community Call yesterday morning, this screenshot combined with the one above illustrates how color coding is kind of confusing, and is inconsistently displayed. The colors are no longer visible when the menu item is selected (ILL Requests, in this case). When you view the item list, there's no color coding or icon to quickly show which items have which status - you have to READ the words (!).
It might be nice to have meaningful icons visible in both the Your Account summary AND next to the status message (Available for Pickup, Cancelled, In transit).
|
@demiankatz Oops, I forgot to update the tests to handle the theme-specific cache. Now fixed! |
Follow-up from #4088. Moves more of the processing server-side for easier customization. Also fixes and adds new tests. Still compatible with the previous mechanism so that any existing customization will continue to work.
TODO