Edgewall Software
Modify

Opened 4 years ago

Closed 21 months ago

#11928 closed defect (fixed)

Format cannot be saved in notifications of preferences

Reported by: Jun Omae Owned by: Jun Omae
Priority: normal Milestone: 1.2.1
Component: notification Version: 1.1.3
Severity: normal Keywords:
Cc:
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.
  • Add get_session_attribute to trac.web.session.

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.

Attachments (1)

T11928_save_notification_format_session_pref.diff (5.3 KB ) - added by Peter Suter 21 months ago.

Download all attachments as: .zip

Change History (24)

comment:1 Changed 4 years ago by 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.)

comment:2 Changed 4 years ago by 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.

comment:3 Changed 4 years ago by Jun Omae

Milestone: 1.1.41.1.5

comment:4 Changed 4 years ago by Ryan J Ollos

Milestone: 1.1.51.1.6

comment:5 Changed 3 years ago by Ryan J Ollos

Milestone: 1.1.61.1.7

comment:6 Changed 3 years ago by Ryan J Ollos

Milestone: 1.1.71.2

Milestone renamed

comment:7 Changed 3 years ago by Ryan J Ollos

Owner: set to Ryan J Ollos
Status: newassigned

comment:8 Changed 3 years ago by Ryan J Ollos

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

comment:9 Changed 22 months ago by 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.

Changed 21 months ago by Peter Suter

comment:10 Changed 21 months ago by 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).

comment:11 Changed 21 months ago by Jun Omae

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 21 months ago by Jun Omae (previous) (diff)

comment:12 Changed 21 months ago by Peter Suter

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

comment:13 Changed 21 months ago by 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).

Last edited 21 months ago by Jun Omae (previous) (diff)

comment:14 in reply to:  13 Changed 21 months ago by Jun Omae

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.

comment:15 Changed 21 months ago by 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

comment:16 Changed 21 months ago by 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?)

comment:17 Changed 21 months ago by Jun Omae

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

comment:18 Changed 21 months ago by 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.

comment:19 in reply to:  18 ; Changed 21 months ago by 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.

comment:20 in reply to:  19 Changed 21 months ago by 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].

comment:21 Changed 21 months ago by 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.

comment:22 Changed 21 months ago by 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.

comment:23 Changed 21 months ago by Jun Omae

API Changes: modified (diff)
Resolution: fixed
Status: assignedclosed

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

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Jun Omae.
The resolution will be deleted.
to The owner will be changed from Jun Omae to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.