I missed something during QA of bug 29723. It appears that the button will appear regardless of whether table_settings is passed into the KohaTable/kohaTable instantiator. This is a bug and the button leads to a server error.
Spotted by Owen when testing bug 31028 * I get an error in the browser console when I click "Configure" on the datatable on the catalog concerns page.
Created attachment 139567 [details] [review] Bug 31435: Tie 'Configure' to table_settings This patch adds a check for table_settings to prevent the display of 'Configure this table' when table settings are not passed to the datatable wrapper.
Created attachment 139570 [details] [review] Bug 31435: Tie 'Configure' to table_settings This patch adds a check for table_settings to prevent the display of 'Configure this table' when table settings are not passed to the datatable wrapper.
Is there a test plan? Maybe a list of tables we should check?
Created attachment 140209 [details] [review] Bug 31435: Tie 'Configure' to table_settings This patch adds a check for table_settings to prevent the display of 'Configure this table' when table settings are not passed to the datatable wrapper.
Created attachment 140210 [details] [review] Bug 31435: (follow-up) Make kohaTable calls consistent Our calls to instantiate a kohaTable from JS were inconsistent in how they pass table_settings where no table_settings exist. This patch replaces empty arrays and empty hashrefs with a consistent 'null' being passed to ake it clear we don't have any settings to pass yet.
Created attachment 140211 [details] [review] Bug 31435: (follow-up) Make KohaTable calls consistent Our calls to instantiate a KohaTable from were inconsistent in how they pass table_settings where no table_settings exist. This patch replaces empty arrays and empty hashrefs with a consistent 'null' being passed to ake it clear we don't have any settings to pass yet.
OK.. so whilst trying to come up with a test plan I spotted a few inconsistencies in how we're instantiating our kohaTables and KohaTables. Instead of not passing anything, or passing an explicit 'null', we were passing empty arrays and empty hashrefs in some places.. my follow-ups correct that for consistency. In short, the tables affected by the two follow-up patches will all have displayed the 'Configure' link even when there's nothing to configure... that's the test plan ;P
In the second patch the file koha-tmpl/intranet-tmpl/prog/en/modules/tools/quotes.tt has undef instead of null. Not sure I agree with the last patch, it is unnecessary information exposed to the programmer, since there is a default, no settings, then I don't see why it would have to be marked explictly, we don't supply the other KohaTable add_filters parameter either. I'd suggest removing the last patch.
In retrospect, I agree with you Joonas.. I wrote the patch as a way to record that I'd gone through all the templates that call KohaTable.. but sending nothing is perfectly OK.. it's sending and empty hash or empty array that's an issue.
Created attachment 140260 [details] [review] Bug 31435: Tie 'Configure' to table_settings This patch adds a check for table_settings to prevent the display of 'Configure this table' when table settings are not passed to the datatable wrapper.
Created attachment 140261 [details] [review] Bug 31435: (follow-up) Make kohaTable calls consistent Our calls to instantiate a kohaTable from JS were inconsistent in how they pass table_settings where no table_settings exist. This patch replaces empty arrays and empty hashrefs with a consistent 'null' being passed to ake it clear we don't have any settings to pass yet.
*cough* test plan *cough* ;)
Created attachment 140265 [details] [review] Bug 31435: Tie 'Configure' to table_settings This patch adds a check for table_settings to prevent the display of 'Configure this table' when table settings are not passed to the datatable wrapper. Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Suggested test plan: * Without patch: * Go to Administration > SMTP servers * Verify the configure button shows, but doesn't work * Apply patch * Reload the SMTP servers page * The configure button will no longer show
Created attachment 140266 [details] [review] Bug 31435: (follow-up) Make kohaTable calls consistent Our calls to instantiate a kohaTable from JS were inconsistent in how they pass table_settings where no table_settings exist. This patch replaces empty arrays and empty hashrefs with a consistent 'null' being passed to ake it clear we don't have any settings to pass yet. Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 140318 [details] [review] Bug 31435: Tie 'Configure' to table_settings This patch adds a check for table_settings to prevent the display of 'Configure this table' when table settings are not passed to the datatable wrapper. Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Suggested test plan: * Without patch: * Go to Administration > SMTP servers * Verify the configure button shows, but doesn't work * Apply patch * Reload the SMTP servers page * The configure button will no longer show Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Created attachment 140319 [details] [review] Bug 31435: (follow-up) Make kohaTable calls consistent Our calls to instantiate a kohaTable from JS were inconsistent in how they pass table_settings where no table_settings exist. This patch replaces empty arrays and empty hashrefs with a consistent 'null' being passed to ake it clear we don't have any settings to pass yet. Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Created attachment 140320 [details] [review] Bug 31435: (QA follow-up) Don't pass explicitly default parameter values For kohaTable these are the default values. Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Pushed to master for 22.11. Nice work everyone, thanks!