Add Notifications CMS functionality #3103
Conversation
|
Thanks for sharing this, @jschultze! I'm currently focusing most of my time on finishing up the 9.1 release, but I will give this a review once development begins on 10.0. |
|
Thanks @demiankatz ! There seems to be a request for additional functions that came up after WOLFcon - hebis jut informed me about that. I will look into it in the next week, and maybe I can include that in the pull request before you start with the review. |
|
Great, thanks, @jschultze! |
There was a problem hiding this comment.
@jschultze, just a couple of very high-level observations from a quick look at the code. Since you said more changes might be coming soon, I haven't given this a deeper review yet, but I thought it might be helpful to at least get a conversation started here. :-)
EreMaijala
requested review from
EreMaijala
and removed request for
EreMaijala
|
Based on discussion with @ThoWagen on today's Community Call, I have given this PR an "on hold" label since I know there is pending work that we expect to have added at some point in the future, and it's probably best to delay review until that is completed. Please let me know when you're ready and I'll be happy to remove the label and resume reviewing. |
|
@demiankatz The functionality is ready and I will start looking into the first observations that you mentioned above. |
…ion to "notifications.Admin". Login is required by default, usernames can be set to restrict the access even more. If the permission is not granted an error message will be displayed.
|
Thanks, @jschultze, I'm removing the "on hold" label and will give this another review soon. I notice that there are some minor conflicts that need to be resolved here. Would you like help resolving these? If so, just let me know and I'll take care of them at the same time I do my review. |
demiankatz
changed the title
There was a problem hiding this comment.
@jschultze, I had a few minutes free today, so I went ahead and resolved the merge conflicts as well as applying some of the standard style fix tools (php-cs-fixer and phpcbf). I didn't have time for a full review, but I skimmed through and raised a few more points for discussion (see below).
| @@ -0,0 +1,6 @@ | |||
| /*! jQuery UI - v1.13.2 - 2023-02-20 | |||
There was a problem hiding this comment.
How critical is jQuery UI to this PR? We are currently in the early stages of removing all jQuery dependencies from the codebase, so I'm reluctant to add a major new dependency if it can be avoided...
There was a problem hiding this comment.
I think it is only used to enable a tab for each configured language in the edit forms for pages and broadcasts. The reordering of entries in the list view to change the displayed order is done with the DataTables library.
Is it just a problem using jQuery UI particularly, so that I can try and find a smaller, independent library for the tabs? Or should additional JavaScript libraries be avoided?
There was a problem hiding this comment.
We already have tabbed interfaces in a couple of places (most notably on the record page) without introducing additional dependencies. I confess that I can't remember exactly how those are implemented, but it might be worth looking for precedent before adding something new. Or, if adding something new is valuable, maybe it should be applied to all tabbed interfaces for consistency.
In any case, I'm certainly not completely opposed to adding a dependency if it's a significant time saver or improves the user experience. I just try to avoid them unless absolutely necessary because they often lead to future technical debt.
There was a problem hiding this comment.
I'm currently trying to refactor the notifications language-tabs to the way tabs are implemented on the record page.
There was a problem hiding this comment.
I did not get the tabs from the detail view to work in the notifications context, so I build a small JS/CSS tab navigation myself. jQuery UI is removed.
The less and sass files are included. Should all the generated files be committed too?
/themes/bootprint3/css/compiled.css
/themes/bootstrap3/css/compiled.css
/themes/bootstrap3/css/vendor/simple-keyboard/index.css
/themes/bootstrap3/js/vendor/chart.js
/themes/bootstrap3/js/vendor/simple-keyboard/index.js
/themes/bootstrap3/js/vendor/simple-keyboard-layouts/index.js
/themes/local_theme_example/css/compiled.css
/themes/sandal/css/compiled.css
There was a problem hiding this comment.
@demiankatz composer update-npm-dependencies (or npm run updateDeps for a single theme) doesn't allow you to do it for a single package and can cause side-effects unless you're careful.
There was a problem hiding this comment.
Good point, @EreMaijala. I guess we need to decide on a strategy here. I think there are two general approaches:
1.) We pin all dependencies, and add composer/npm tasks to help identify pinned dependencies that are eligible for upgrades.
2.) We leave dependencies unpinned, and I open a separate PR to upgrade and test everything, and then we merge that separately to remove extraneous changes from this PR.
Any preference?
There was a problem hiding this comment.
For now, I've opened #3522 to bring things up to date independently of this PR... but I'm still open to discussing long term strategy independent of that. :-)
There was a problem hiding this comment.
@demiankatz I don't have a strong preference, but pinning would be consistent with composer dependencies and would avoid the situation here where just trying to add or update one dependency will easily cause side-effects.
There was a problem hiding this comment.
It occurs to me that the best solution might be to start committing the lock file. Can you think of any reason not to?
There was a problem hiding this comment.
It's fixed!! I've set up a couple of broadcast messages, and when I click the edit icon I see that all of the elements of the broadcast are available to be edited, including the dates!
I tested changing the dates, and the new dates saved correctly. As far as I can tell, all is working properly now. Thanks, @maccabeelevine and @jschultze!
|
Thanks for the test, @sturkel89 I will start with to update the code to the current version of VuFind. |
# Conflicts: # composer.json # composer.lock # module/VuFind/src/VuFind/Db/Table/PluginManager.php # themes/bootprint3/css/compiled.css # themes/bootstrap3/css/compiled.css # themes/bootstrap3/js/vendor/simple-keyboard-layouts/index.js # themes/bootstrap3/package.json # themes/bootstrap3/templates/admin/menu.phtml # themes/bootstrap3/theme.config.php # themes/local_theme_example/css/compiled.css # themes/sandal/css/compiled.css # themes/sandal/less/search.less # themes/sandal/scss/search.scss
|
@demiankatz I merged the current dev branch into this pull request and added support for the bootstrap5 theme. |
There was a problem hiding this comment.
I reviewed this PR with special consideration of the updates to the new themes.
FRONT PAGE ISSUES
Notification banners that are supposed to appear on the front page are NOT functional in the new themes, although they appear normal in the legacy themes.
bootstrap3:
sandal:
In the new themes, the banners DO NOT APPEAR on the front page, at all.
sandal5:
OTHER PAGES ISSUES
Notifications banners appear in both legacy and new themes in VuFind subpages (i.e., item pages, results list pages) but their formatting is different between legacy and new themes.
Notification banners stretch across the screen in the legacy themes:
bootstrap3:
sandal:
In both new themes, the banners appear in the top corner, pushing the Theme and Language dropdowns to the middle of the screen.
bootstrap5:
sandal5:
Also, the other issues I noted in my earlier reviews are still problems:
- I can't get any popover explanations for some of the icons on the management page (eyeball, globe)
- Clicking the X in the notification banner does not make it disappear.
NOTIFICATIONS PAGES
Notifications pages are functional, in that the text links to the notifications pages appear on the home page and the other pages in all themes.
The locations of the links are a little awkward in the new themes. In the legacy themes, the links are aligned with the word or icon for VuFind. In the new themes, they are in the middle of the page but above the VuFind.
bootstrap3:
Same browser, bootstrap5:
sandal:
Same browser, sandal5:
With narrower screens, the break points for hiding the notification page links inside the hamburger menu are different between legacy and new themes. There are many different breaking points and variations, so I didn't document them.
# Conflicts: # composer.lock # themes/bootprint3/css/compiled.css # themes/bootstrap3/css/compiled.css # themes/bootstrap3/less/mixins/mixins.less # themes/bootstrap3/package.json # themes/bootstrap3/scss/mixins/mixins.scss # themes/bootstrap3/templates/admin/menu.phtml # themes/bootstrap3/templates/header.phtml # themes/bootstrap3/templates/search/home.phtml # themes/bootstrap3/theme.config.php # themes/bootstrap3/tools/copyDependencies.mjs # themes/bootstrap5/css/compiled.css # themes/bootstrap5/templates/admin/menu.phtml # themes/bootstrap5/templates/header.phtml # themes/bootstrap5/theme.config.php # themes/local_theme_example/css/compiled.css # themes/sandal/css/compiled.css # themes/sandal5/css/compiled.css
|
@demiankatz @sturkel89 The display problems with the new themes should be resolved. |
|
Thanks, @jschultze! We'll test again soon; in the meantime, I noticed that the build was failing because of some slightly out-of-order translations, so I ran the |
|
Looks like the build is now failing because of the AdminMenuTest; I'm guessing that you may have added a new menu item, so the test needs to be adjusted to expect it. |
|
@demiankatz The test for the number of menu items is fixed. I get three remaining errors locally, all in the |
|
Thanks, @jschultze -- the tests are all passing now. It looks like the build is now failing due to some minor style issues here and there detected by phpcbf (mostly long lines and missing comments). We'll need to address those before this can be merged, but it's not urgent. In the meantime, @sturkel89 can resume testing as time permits! |
This pull request adds the Notifications mini-CMS functionality to VuFind. It is developed by effective WEBWORK GmbH in cooperation with hebis. Notifications allow the creation of simple, CMS-like "pages" that can be accessed via a new menu in the header area. Also short information "broadcasts" can be displayed in the header area and on the search home.
Markdown is used to compose the content, and different translations for all currently enabled languages are possible. Pages and broadcasts are edited from the Admin-area of DSpace.
TODO