Edgewall Software

Opened 9 years ago

Last modified 7 years ago

#11928 closed defect

Format cannot be saved in notifications of preferences — at Version 14

Reported by: Jun Omae Owned by: Jun Omae
Priority: normal Milestone: 1.2.1
Component: notification Version: 1.1.3
Severity: normal Keywords:
Cc: Branch:
Release Notes:
  • Fix unable to save notification format with no subscription rules in prefs/notification.
  • Add Default format in prefs/notification.
  • Add [notification] default_format.email option.
API Changes:

Add notification.format.<transport> in user's session.

Internal Changes:

Description

Installed th:TracHtmlNotificationPlugin, selected text/html format and saved the changes. However, the selection is reverted to text/plain.

After adding something to subscription rules, selected format could be saved.

Change History (15)

comment:1 by Peter Suter, 9 years ago

Yes, there is a fundamental mismatch between UI and storage model.

The preferences panel UI pretends there is just one format preference setting (per distribution transport).

But the setting is actually stored in every / any subscription. So if there is no subscription it is not stored.

Default subscriptions currently always use a hardcoded format. #11875 would make these configurable.

The preferences UI could be changed so the preferred format can / must be selected separately for each subscription.

Alternatively, the storage model could be changed. (The interfaces would also have to be adjusted accordingly.)

comment:2 by Peter Suter, 9 years ago

Replying to jomae in #11934:

  1. Related to #11928, I am not sure whether a user wants to configure format per subscriber. It might be enough to configure format per transport like this.
    • default format per user: req.session['notification.default_format.email'] = 'text/html'
    • default format for all users: default_format.email = text/html in trac.ini.

OK, that seems reasonable. I agree configuring text/html vs. text/plain per subscriber does not seem very useful.

Maybe more useful could be special-case formats and formatters for certain providers, like secure minimal text/plain and SecurityFlagSubscriber, or full backup text/html and DeleteSubscriber?

We could remove the format column from table notify_subscription, but keep format in the INotificationSubscriber. Most subscribers would retrieve it from req.session as you proposed, and special subscribers can return special formats.

comment:3 by Jun Omae, 9 years ago

Milestone: 1.1.41.1.5

comment:4 by Ryan J Ollos, 9 years ago

Milestone: 1.1.51.1.6

comment:5 by Ryan J Ollos, 9 years ago

Milestone: 1.1.61.1.7

comment:6 by Ryan J Ollos, 9 years ago

Milestone: 1.1.71.2

Milestone renamed

comment:7 by Ryan J Ollos, 8 years ago

Owner: set to Ryan J Ollos
Status: newassigned

comment:8 by Ryan J Ollos, 8 years ago

Milestone: 1.2next-major-releases
Owner: Ryan J Ollos removed
Status: assignednew

comment:9 by Massimo <massimo.b@…>, 7 years ago

There is no owner anymore. Are there plans to fix this within 1.2? Configuring text/html vs. text/plain per subscriber is very useful, as people have different taste about that. And this actually works at least if some custom rule is added additional, considering this issue as a bug.

comment:10 by Peter Suter, 7 years ago

It seems this ticket causes frequent confusion to users (e.g. on Mailing list 1, 2).

Maybe the attached patch is an improvement? It stores the preferred format in the session (still one format per transport, in addition to still storing it also with each subscription. So only for default subscriptions we need to check the notified user's session for the preferred format (or fall back to the default format from [notification-subscriber] if the user did not set one).

comment:11 by Jun Omae, 7 years ago

Applied your patch to 1.2-stable, revised changes and added unit tests: jomae.git@t11928+1.2

Comments:

  • I don't think it it good to use get_preferred_format() in each subscriber.
  • Adding [notification] default_format.email option would be able to edit the value via TracIniAdminPanelPlugin and IniAdminPlugin. I think it is not easy to edit [notification-subscriber] section.
  • I consider it might be good to use notification.format.<transport> rather than notification.default_format.<transport>. I think the notification.default_format.<transport> is verbose a bit.
Last edited 7 years ago by Jun Omae (previous) (diff)

comment:12 by Peter Suter, 7 years ago

Thanks, your improvements look much better! I also agree with your comments.

comment:13 by Jun Omae, 7 years ago

Added unit tests for prefs/notification in jomae.git@t11928+1.2. I noticed anyone can delete and change priority of non-owned subscription rules in notification panel (not fixed yet in the branch).

Last edited 7 years ago by Jun Omae (previous) (diff)

in reply to:  13 comment:14 by Jun Omae, 7 years ago

API Changes: modified (diff)
Milestone: next-major-releases1.2.1
Owner: set to Jun Omae
Release Notes: modified (diff)
Status: newassigned

I noticed anyone can delete and change priority of non-owned subscription rules in notification panel (not fixed yet in the branch).

Fixed the issue in [a024c1dd2/jomae.git] (jomae.git@t11928+1.2). I'll push the changes to 1.2-stable if no objections.

Note: See TracTickets for help on using tickets.