Closed (fixed)
Project:
Feeds
Version:
8.x-3.x-dev
Component:
Code
Priority:
Normal
Category:
Bug report
Assigned:
Unassigned
Reporter:
Created:
3 Mar 2024 at 13:45 UTC
Updated:
2 Jun 2024 at 14:44 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #4
franzEslint is passing now.
Comment #5
franzPassing both now.
Comment #6
megachrizThanks for helping! Unfortunately, the JS in modules/log/js/feed_type_settings.js is no longer doing what it should do.
In the vertical tab 'Log settings' on a feed type page it should tell the user whether or not logging is enabled for the feed type:
After the changes from this issue, the summary on the vertical tab 'Log settings' always says that logging is disabled, even if it is enabled.

Comment #7
franzI fixed that portion of code that was "vanillaed", it seems to be working now.
Comment #8
ptmkenny commentedI just tested this and "Logging disabled" and "Logging enabled" are reflected properly.
However, I noticed an unusual behavior that I would like to confirm.
With logging enabled, when both "Log message" and "Log item" are checked, if you uncheck "Log message" first, then "Log item" is disabled and you cannot uncheck it.
However, if you uncheck "Log item" first, then you can uncheck "Log message" as well.
This is inconsistent and does not make sense to me.
However, this behavior seems to be the same on the 3.x branch of the module, so it may be a separate issue.
Not marking RTBC because of the above concern.
Comment #10
megachriz@ptmkenny
But after you save the settings, "Log item" will be unchecked. The idea is that if you disable logging for a certain operation, you cannot log items for it. So that's why the "Log item" checkbox will become disabled.
I do see that there's an issue that when both checkboxes are not checked on page load, when you enable logging for a certain operation, the "Log item" checkbox remains disabled. But when unchecking and checking logging for an operation, you can control "Log item" again.
I think changing the form for the log settings is something that would better be addressed in a follow-up issue, but thanks for testing @ptmkenny :)
I merged the code.
Comment #11
ptmkenny commented@MegaChriz Thanks for the explanation!