Bug #1064
Merge thread may duplicate subscriptions
| Status: | Closed | Start date: | 07/04/2010 | |
|---|---|---|---|---|
| Priority: | Normal | Due date: | ||
| Assignee: | Tom Moore | % Done: | 100% |
|
| Category: | Moderation | |||
| Target version: | 1.6.1 | |||
| Reproducibility: | Once | Database Type: | ||
| Reported In MyBB Version: | 1.4.13 | Database Version: | ||
| PHP Version: | SQA assignments: | Stefan T. | ||
| Browser: |
Description
I don't know whether you want to consider this a bug or not.
When merging threads, thread subscriptions of the old thread are simply updated to point to the new thread. This may cause duplicate subscriptions (in the subscriptions table) if a user is subscribed to both threads. This doesn't appear to exactly have any adverse effects (haven't looked everywhere), though it can be questionable as to whether which subscription is valid, especially if they have different types.
(ideally, there probably should be a unique key placed on uid,tid of the subscriptions table)
Associated revisions
Fixes Merge thread may duplicate subscriptions (fixes:1064)
Fixes Merge thread may duplicate subscriptions (fixes:1064)
History
Updated by Michael Malin over 1 year ago
- Status changed from New to Confirmed
Updated by Tom Moore over 1 year ago
I'm not too sure about having a unique key on the subscriptions table. For example, if the user has email notifications for one thread, but no notifications for the other - you could assume they'd still like to receive emails as part of their subscription. In the instances of dual subscriptions, email should take priority I think...
Updated by zinga burga over 1 year ago
Tom Moore wrote:
I'm not too sure about having a unique key on the subscriptions table. For example, if the user has email notifications for one thread, but no notifications for the other - you could assume they'd still like to receive emails as part of their subscription. In the instances of dual subscriptions, email should take priority I think...
Data integrity 101?
Either case, you need to really define what it means if there are two subscriptions for a tid and uid. If the email one is taking preference, then what's the point in keeping the other row?
Updated by Tom Moore over 1 year ago
Remove it? I thought that was self explanatory...
Updated by Ryan Gordon over 1 year ago
If you add a unique key then all you will get is unique constraint SQL errors... Which will make the problem worse.
The best thing to do is to do a "select distinct" in the query i think.
Updated by zinga burga over 1 year ago
Ryan Gordon wrote:
If you add a unique key then all you will get is unique constraint SQL errors... Which will make the problem worse.
From a data integrity standpoint, it's better to error out than to have junk data. Erroring out means that you catch more problems. Being silent about everything gives a false impression things are going well.
That's the reason why so many enterprise class systems are quite pedantic on data constraints.
Updated by Ryan Gordon over 1 year ago
zinga burga wrote:
Ryan Gordon wrote:
If you add a unique key then all you will get is unique constraint SQL errors... Which will make the problem worse.
From a data integrity standpoint, it's better to error out than to have junk data. Erroring out means that you catch more problems. Being silent about everything gives a false impression things are going well.
That's the reason why so many enterprise class systems are quite pedantic on data constraints.
Okay then but it still isn't a solution to this problem.
The fact is it is allowed to have more then one uid in the subscriptions table
Updated by zinga burga over 1 year ago
Ryan Gordon wrote:
zinga burga wrote:
Ryan Gordon wrote:
If you add a unique key then all you will get is unique constraint SQL errors... Which will make the problem worse.
From a data integrity standpoint, it's better to error out than to have junk data. Erroring out means that you catch more problems. Being silent about everything gives a false impression things are going well.
That's the reason why so many enterprise class systems are quite pedantic on data constraints.
Okay then but it still isn't a solution to this problem.
The fact is it is allowed to have more then one uid in the subscriptions table
It isn't a solution, it was just a suggestion to help potentially discover future bugs (and possibly any plugin that decides to insert dupe entries).
It may assist in a solution anyway (eg for MySQL an INSERT IGNORE statement) but probably not one that works across the supported database systems.
I generally leave implementation details to you guys.
Updated by Tom Moore over 1 year ago
- Status changed from Confirmed to Assigned
- Assignee set to Tom Moore
- Target version set to 1.6.1
Preparing patch.
The fix is to check subscriptions for both the source thread and destination thread. If there is a subscription for both, the source thread subscription is removed and destination thread kept with no changes to preferences.
If there is a source thread subscription, but no destination thread subscription, the subscription is updated with the new tid - with no changes to preferences too. The user can easily change this is they want when they receive a notification.
Updated by Tom Moore over 1 year ago
- Status changed from Assigned to Resolved
- % Done changed from 0 to 100
Applied in changeset r5212.
Updated by zinga burga over 1 year ago
Tom Moore wrote:
Applied in changeset r5212.
I haven't double checked this, but your patch appears to assume that there's only one user.
Example, user 1 is subscribed to thread A, and user 2 is subscribed to thread B. Merge A into B, and I would assume that user 1's subscription gets deleted.
Updated by Tom Moore over 1 year ago
- Status changed from Resolved to Feedback
Yeah I missed that(!).
Preparing patch.
Updated by Tom Moore over 1 year ago
- Status changed from Feedback to Resolved
Applied in changeset r5216.
Updated by Stefan T. over 1 year ago
- Status changed from Resolved to Closed
- SQA assignments set to Stefan T.