Edgewall Software
Modify

Opened 5 years ago

Closed 3 years 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: 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.
  • 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 3 years ago.

Download all attachments as: .zip

Change History (24)

comment:1 by Peter Suter, 5 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, 5 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, 5 years ago

Milestone: 1.1.41.1.5

comment:4 by Ryan J Ollos, 5 years ago

Milestone: 1.1.51.1.6

comment:5 by Ryan J Ollos, 4 years ago

Milestone: 1.1.61.1.7

comment:6 by Ryan J Ollos, 4 years ago

Milestone: 1.1.71.2

Milestone renamed

comment:7 by Ryan J Ollos, 4 years ago

Owner: set to Ryan J Ollos
Status: newassigned

comment:8 by Ryan J Ollos, 4 years ago

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

comment:9 by Massimo <massimo.b@…>, 3 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, 3 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, 3 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 3 years ago by Jun Omae (previous) (diff)

comment:12 by Peter Suter, 3 years ago

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

comment:13 by Jun Omae, 3 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 3 years ago by Jun Omae (previous) (diff)

in reply to:  13 comment:14 by Jun Omae, 3 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.

comment:15 by Ryan J Ollos, 3 years ago

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 by Peter Suter, 3 years ago

(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 by Jun Omae, 3 years ago

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

comment:18 by Ryan J Ollos, 3 years ago

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

in reply to:  18 ; comment:19 by Jun Omae, 3 years ago

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.

in reply to:  19 comment:20 by Jun Omae, 3 years ago

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 by Ryan J Ollos, 3 years ago

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 by Jun Omae, 3 years ago

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 by Jun Omae, 3 years ago

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. Next status will be 'reopened'.
to as closed 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.