'Connection failed' is a bit strong for the case where RabbitMQ is not found. In reality, we fallback to polling the database so Koha will function reasonably without the overhead of RabbitMQ. We should clarify that on the about page.
Created attachment 116575 [details] [review] Bug 27661: Update about for message broker status This patch updates the about page with regards to the message broker status and adds some small formatting improvements for displaying the status. Test plan 1/ Run through the yarn build process to update the CSS file. 2/ View the about page without RabbitMQ installed 3/ View the about page with RabbitMQ installed 4/ Note the formatting and message change and signoff
Created attachment 116848 [details] [review] Bug 27661: Update about for message broker status This patch updates the about page with regards to the message broker status and adds some small formatting improvements for displaying the status. Test plan 1/ Run through the yarn build process to update the CSS file. 2/ View the about page without RabbitMQ installed 3/ View the about page with RabbitMQ installed 4/ Note the formatting and message change and signoff Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 116855 [details] [review] Bug 27661: Update about for message broker status This patch updates the about page with regards to the message broker status and adds some small formatting improvements for displaying the status. Test plan 1/ Run through the yarn build process to update the CSS file. 2/ View the about page without RabbitMQ installed 3/ View the about page with RabbitMQ installed 4/ Note the formatting and message change and signoff Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 116856 [details] [review] Bug 27661: (QA follow-up) Capitalization Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
+.status_fatal { + background-color: #FF6347 !important; Why tomato? :) We already have .error { color: #CC0000; }
This change is actually not advertised by the patch and not directly linked with what we are doing here. Can we remove it from the patch?
(In reply to Jonathan Druart from comment #5) > +.status_fatal { > + background-color: #FF6347 !important; > > Why tomato? :) > > We already have > .error { > color: #CC0000; > } Fair point, I had forgotten we had 'error' already.. kinda annoying this table has it's own classing in the first place to be honest. Would you prefer a css change to fix the color to match our .error class.. or wider template followup to convert to using the existing classes.. I'm thinking the latter.
Hmm, I find the stark red as a background a bit challenging to read.. that's probably why I went for Tomato.. Must admit, I find the inconsistency in the about page upsets my OCD.. some status's are on their own line, others are inline.. some have background colors applied to a span, whilst others still have the whole row highlighted.
Created attachment 117006 [details] [review] Bug 27661: (QA follow-up) Use bootstrap classes This patch removes the local status_* css rules in favour of using the bg-* rules that are shipped with the bootstrap version we are using and then updates the relevant lines in the template to use those classes. We also update the default table styling which adds background color to the odd table rows so that our local classes take precidence.
Setting back to signed off to attract a second QA run here.. I think the final followup gives us a much cleaner approach to the styling which will allow for more global use of the bootstrap bg-* options for tables.
https://getbootstrap.com/docs/3.4/css/#helper-classes-backgrounds for reference of the bootstrap classes I'm talking about
Created attachment 117010 [details] [review] Bug 27661: (QA follow-up) Use bootstrap classes This patch removes the local status_* css rules in favour of using the bg-* rules that are shipped with the bootstrap version we are using and then updates the relevant lines in the template to use those classes. We also update the default table styling which adds background color to the odd table rows so that our local classes take precidence. Signed-off-by: Owen Leonard <oleonard@myacpl.org>
We are advertizing here that we support SQL polling and Rabbit MQ. Is that really the way we want to go? Support both ways for any new case where we would like to use RabbitMQ? The SQL polling has been added to the initial use of RabbitMQ as a fallback and probably for building some confidence. Since I doubt that we will be supporting both ways, I would propose rewording. Any thoughts?
(In reply to Marcel de Rooy from comment #13) > We are advertizing here that we support SQL polling and Rabbit MQ. > Is that really the way we want to go? Support both ways for any new case > where we would like to use RabbitMQ? > The SQL polling has been added to the initial use of RabbitMQ as a fallback > and probably for building some confidence. > > Since I doubt that we will be supporting both ways, I would propose > rewording. > Any thoughts? See misc/background_jobs_worker.pl lines 58-63, there is no additional code, and it applies to all jobs. It shouldn't hurt to have it and keep it.
(In reply to Jonathan Druart from comment #14) > (In reply to Marcel de Rooy from comment #13) > > We are advertizing here that we support SQL polling and Rabbit MQ. > > Is that really the way we want to go? Support both ways for any new case > > where we would like to use RabbitMQ? > > The SQL polling has been added to the initial use of RabbitMQ as a fallback > > and probably for building some confidence. > > > > Since I doubt that we will be supporting both ways, I would propose > > rewording. > > Any thoughts? > > See misc/background_jobs_worker.pl lines 58-63, there is no additional code, > and it applies to all jobs. It shouldn't hurt to have it and keep it. Thx for clarifying. Had the impression that it was written for a specific job. Which is not the case. Revisiting.
Created attachment 117022 [details] [review] Bug 27661: Use td.bg-success for ES and memcached as well
Created attachment 117023 [details] [review] Bug 27661: Update about for message broker status This patch updates the about page with regards to the message broker status and adds some small formatting improvements for displaying the status. Test plan 1/ Run through the yarn build process to update the CSS file. 2/ View the about page without RabbitMQ installed 3/ View the about page with RabbitMQ installed 4/ Note the formatting and message change and signoff Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 117024 [details] [review] Bug 27661: (QA follow-up) Capitalization Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 117025 [details] [review] Bug 27661: (QA follow-up) Use bootstrap classes This patch removes the local status_* css rules in favour of using the bg-* rules that are shipped with the bootstrap version we are using and then updates the relevant lines in the template to use those classes. We also update the default table styling which adds background color to the odd table rows so that our local classes take precidence. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 117026 [details] [review] Bug 27661: Use td.bg-success for ES and memcached as well Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
(In reply to Marcel de Rooy from comment #13) > We are advertizing here that we support SQL polling and Rabbit MQ. > Is that really the way we want to go? Support both ways for any new case > where we would like to use RabbitMQ? > The SQL polling has been added to the initial use of RabbitMQ as a fallback > and probably for building some confidence. > > Since I doubt that we will be supporting both ways, I would propose > rewording. > Any thoughts? I think it would be trivial to drop update the change here at the same time as deprecating the polling fallback should we do that in the future. I'm actually wondering if things might go the other way... I was wary of adding yet another system to run and maintain when we added RabbitMQ personally.. I'm really not sure we need all that it offers and personally would have preferred an approach where we started with an SQL based queue and migrated to a bigger broker only as and when we found we needed it/found that polling was a performance issue. I'd also like to drop the 'required' state of the STOMP dependency as we don't actually require it to function.. we just require it at the moment as we use it to work out if RabbitMQ is present in the worker daemon.
Nice addition also using the classes for ES and Memcached :)
Pushed to master for 21.05, thanks to everybody involved!
Pushed to 20.11.x for 20.11.04
Missing dependency, not backported to 20.05