Format cannot be saved in notifications of preferences

[Jun Omae]

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.

[Peter Suter]

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.)

[Peter Suter]

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.

[Ryan J Ollos]

Milestone renamed

[Massimo <massimo.b@…>]

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.

[Peter Suter]

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).

[Jun Omae]

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

• [1034002e6/jomae.git] verify format value in prefs/notification
• [015b2615c/jomae.git] add Default format to prefs/notification
• [00fc28466/jomae.git] add unit tests for notification.default_format.<transport> in session and [notification] default_format.<transport> option
• [b3c830448/jomae.git] add [notification] default_format.email option
• [f5ed5f8b3/jomae.git] move a use of get_preferred_format from each subscriber to NotificationSystem.distribute_event()
• [df3e3961b/jomae.git] remove req.session.save(), which is automatically called
• [6c815d23b/jomae.git] s/_format/format_
• [c455774f9/jomae.git] applied T11928_save_notification_format_session_pref.diff

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.

[Peter Suter]

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

[Jun Omae]

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).

[Jun Omae]

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.

[Ryan J Ollos]

I haven't looked at all the changes yet, but get_preferred_format could be refactored to the Trac style (untested):

def get_preferred_format(env, sid, authenticated, transport, default=None):
    key = 'notification.default_format.%s' % transport
    for result, in self.env.db_query("""
            SELECT value
            FROM session_attribute
            WHERE sid=%s
             AND authenticated=%s
             AND name=%s
        """, (sid, 1 if authenticated else 0, key))
        return result
    else:
        return default

[Peter Suter]

(I basically copied get_preferred_format from SessionEmailResolver.get_address_for_session​, so you might also want to refactor that. Maybe even add a generic version in trac.web.session or on env?)

[Jun Omae]

Thanks. I added get_session_attribute to trac.web.session in [2f9c92e3b/jomae.git]. Thoughts?

[Ryan J Ollos]

All changes look good to me. In preferences, I'll add the default value for Default format as part of changes for #12213.

[Jun Omae]

Replying to Ryan J Ollos:

All changes look good to me.

Thanks for the reviewing!

In preferences, I'll add the default value for Default format as part of changes for #12213.

That's good idea. Implemented in [a14ea7f6c/jomae.git]. I've considered I would add hint for the default format.

[Jun Omae]

Replying to Jun Omae:

Replying to Ryan J Ollos:

In preferences, I'll add the default value for Default format as part of changes for #12213.

That's good idea. Implemented in [a14ea7f6c/jomae.git]. I've considered I would add hint for the default format.

Ah. I noticed #12213 is targeting 1.3.x. Reverted only the showing default in [424e1e67a/jomae.git].

[Ryan J Ollos]

I wouldn't object to retargeting #12213 to 1.2.x. Also, the change in [a14ea7f6c/jomae.git] could just precede the other similar changes that are coming in #12213, even if those other changes don't come until 1.3.x.

[Jun Omae]

I think we should consistently modify about showing default value in prefs. I'm going to push the changes without showing default value in this ticket. Also, I'll change milestone of #12213 to 1.2.x.

[Jun Omae]

Committed in [15670] and merged in [15671].