View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 5773 | Composr | actionlog | public | 2024-05-21 19:14 | 2024-08-04 22:57 |
| Reporter | PDStig | Assigned To | PDStig | ||
| Priority | high | Severity | feature | ||
| Status | resolved | Resolution | fixed | ||
| Product Version | 11.alpha4 | ||||
| Summary | 5773: Site messaging JSON is broken | ||||
| Description | The feature to specify multiple site messages using JSON is broken. site.php is not properly reading the JSON and thus throwing errors about messages key not existing in the array (even if it exists in the JSON), and type errors. Also, documentation in tut_news about syntax is wrong and needs updating. | ||||
| Additional Information | See comments about an alternative solution | ||||
| Tags | Roadmap: Over the horizon, Roadmap: v11 | ||||
| Attach Tags | |||||
| Time estimation (hours) | |||||
| Sponsorship open | |||||
|
|
Automated message: This issue was created using the Report Issue Wizard on the Composr homesite. |
|
|
I don't like the idea of this field using text (+ the other options) or JSON (which ignores the other options). This is not consistent with how we use configuration in other areas of the site. And it can be confusing on a UI level. Plus it's incredibly easy to save invalid JSON. Sure, an error is displayed, but it still saves. It should error completely (but then this becomes a problem in of itself because there are other config options on the same page that could be changed). Instead, we should probably strip this config option out into its own XML screen (like field filters / advanced banning). Provide a commented example in the XML. This would also allow us to expand on its functionality in the future if we want, for example allowing to specify specific pages to show a message (this could be very handy for staff use. For example, staff could have a to-do message display to remind others to sync comcode pages to git after actualising an add or edit). Or alternatively, make this a UI module with the ability to add/edit/delete messages individually. Make it its own addon and call it "site_messaging" (I don't think it needs to be core (but it should be bundled); I see no reason why a user shouldn't be able to uninstall it). Make site messages resource meta-aware in resource_fs so they can also be automated. Add a privilege to define who can add/edit site messages, defaulting to staff only. Update documentation accordingly to explain this small module/addon. |
|
|
Automated response: Site messaging is broken; refactor to addon Site messaging was broken when trying to specify multiple messages. This hotfix guts the old site messaging system and removes the configuration options, then adds a new addon called 'site_messaging'. Site messages are now handled through their own screen under Admin Zone > Setup > Site messaging. I decided to go this route because I did not like the previous implementation where there were fields for a single message, but then to specify multiple messages, you had to use JSON (which is easy to get wrong) inside the Comcode-type field. This did not seem appropriate from a contextual and validation point of view. This also adds on the ability to put messages on specific pages by specifying page-links. |
|
|
Okay. This sounds good. However, I do want to add some nuance/context... There's always a tradeoff between quality and where we choose to put our resources. We need to have a baseline of quality, but less important/commonly-used features may reasonably implemented in a more clunky way than more important ones. Hand-writing JSON is an example of that. But given there were apparently insufficient guard rails, probably we'd either have needed to add those, moved the feature to a hidden one, or did it the idealized way anyway. So no complaints from me. |
|
|
That's exactly what my line of thinking was; I figured the amount of time necessary to refactor the JSON validation code to work properly would have not justified doing so compared to making it a proper addon. |
| Date Modified | Username | Field | Change |
|---|---|---|---|
| 2024-05-21 19:20 | PDStig | Note Added: 0008778 | |
| 2024-05-21 19:21 | PDStig | Description Updated | |
| 2024-05-21 19:21 | PDStig | Additional Information Updated | |
| 2024-05-21 19:22 | PDStig | Tag Attached: Roadmap: v11 | |
| 2024-05-21 19:22 | PDStig | Tag Attached: Roadmap: Over the horizon | |
| 2024-05-21 19:24 | PDStig | Note Edited: 0008778 | |
| 2024-05-21 19:27 | PDStig | Note Edited: 0008778 | |
| 2024-05-21 19:27 | PDStig | Note Edited: 0008778 | |
| 2024-05-21 19:29 | PDStig | Note Edited: 0008778 | |
| 2024-05-21 19:29 | PDStig | Note Edited: 0008778 | |
| 2024-05-21 19:31 | PDStig | Note Edited: 0008778 | |
| 2024-05-21 19:32 | PDStig | Note Edited: 0008778 | |
| 2024-05-23 21:57 | PDStig | File Deleted: hotfix-5773, 2024-05-23 9pm.tar | |
| 2024-08-04 22:28 | Chris Graham | Note Added: 0009091 | |
| 2024-08-04 22:57 | PDStig | Note Added: 0009099 |
