Dealbreaker: More topics may be posted, but you won't receive more email notif..

Started by TomasM, July 24, 2018, 11:14:03 AM

Previous topic - Next topic

TomasM


Illori

well, did you have it set like described? if so does it work as expected?

TomasM

Yes, I included the screenshot of my settings in the first few posts. It worked as I would not expect, but it looks that it is a long standing bug.

Arantor

I don't think it's a long standing bug per se - because it certainly worked correctly in 2.0.

I think it's the result of the notifications UI being overhauled and not thoroughly tested.

Kindred

well, it definitely COULD be hacked/modded...  I don't know how easy the mod would be though.
IIRC the notification code is complicated.

Code (In Post.php) Select

// Notify members of a new post.
function notifyMembersBoard


but I'm not certain form just a quick look, which variable or code to change....



Incidentally, looking though, I just realized that the admin CAN turn off the ability to set a notification/watch....
   Request notification on replies          
   Request notification on new topics
settable by group permissions.





I'm still not sure it's a bug so much as a misunderstanding -- if it's a bug, it has existed since 1.1.x, since this is the way it's worked for as long as I've used the software.  (and you say it worked correctly in 2.0, arantor - but this is the way it always worked in 2.0, to the best of my memory.)
Слaва
Украинi

Please do not PM, IM or Email me with support questions.  You will get better and faster responses in the support boards.  Thank you.

"Loki is not evil, although he is certainly not a force for good. Loki is... complicated."

Arantor

If that was the case, what is the difference between "Instantly" and "Instantly (but only for the first unread reply)" as observable on this site?

Kindred

I'm not sure... but I am fairly sure that this is the way it has been working for years, if not more.
Слaва
Украинi

Please do not PM, IM or Email me with support questions.  You will get better and faster responses in the support boards.  Thank you.

"Loki is not evil, although he is certainly not a force for good. Loki is... complicated."

TomasM

I opened the ticket at:

https://github.com/SimpleMachines/SMF2.1/issues/4869

I looked at CreatePost-Notify.php and it looks that code only foresees this one action: notify once. Could anyone suggest how to change the code, so all new topics would be notified?

// A new topic in a watched board then?
elseif ($type == 'topic')
{
$pref = !empty($prefs[$member]['board_notify_' . $topicOptions['board']]) ? $prefs[$member]['board_notify_' . $topicOptions['board']] : (!empty($prefs[$member]['board_notify']) ? $prefs[$member]['board_notify'] : 0);

$content_type = 'board';

$message_type = !empty($frequency) ? 'notify_boards_once' : 'notify_boards';
if (!empty($prefs[$member]['msg_receive_body']))
$message_type .= '_body';
}



TomasM

Hi, guys,

really need your help  :-\

Nobody responded yet to my post here or on the github, to take this job even for the fee. I also privately contacted several SMF devs, but they don't want to take the job too. I have the feeling that it is much deeper burred in the core, so only experienced SMF devs can actually work on it.

Can anyone recommend SMF developers, so I could try to reach them out, hoping they would have reasonable price tag and that they would be able to make this fix/improvement, so whole SMF community would benefit if it would be implemented with admin/user option.

Thank you!

Arantor

No one is interested? Mind you, getting anyone to even acknowledge this as a bug is hard even though it clearly is.

I'd do it but given that recently I've had people telling me things are hard, then going mysteriously quiet when I offered a solution, so I decided not to bother actually submitting any more code to 2.1, and I certainly wouldn't take your money without being 100% confident of it becoming core. (Honestly I probably wouldn't take your money period, but that's another story.)

TomasM

I would love to see this as part of the core!

I'm between two hard places now. I migrated whole community to SMF, but because members don't get regular notifications, they are going back to old forum, so I'm in desperate need to be able finish the transition.

There is nothing wrong to be compensated for your job! ;)

Arantor


Aleksi "Lex" Kilpinen

I would call it a bug, and so it's good it's already on github. Should definitely be fixed, since the UI gives an appearance of having choices, yet they do not work like one would expect them to.
Slava
Ukraini!
"Before you allow people access to your forum, especially in an administrative position, you must be aware that that person can seriously damage your forum. Therefore, you should only allow people that you trust, implicitly, to have such access." -Douglas

How you can help SMF

vii

TomasM, I took a look at CreatePost-Notify.php and I believe I identified the exact problem:

In my copy of CreatePost-Notify.php, around or at Line 119 is the following conditional statement:


            if ($frequency > 2 || (!empty($frequency) && $data['sent']) || in_array($member, $done_members)
                || (!empty($this->_details['members_only']) && !in_array($member, $this->_details['members_only'])))
                continue;


(!empty($frequency) && $data['sent'])

^ In italics is the relevant part. 'sent' (*_log_notify in database) is changed from 0 to 1 when the user has an outstanding notification for a board/topic, and it's flipped back to 0 when they read that board/topic. The problem here is that the code assumes that ANY notification frequency under <= 2 should have the 'sent' value checked, when the only option that (I think) should have that checked is 2 - aka, "Straight away (but only for the first unread message)". The correct fix here is to make sure it checks the value of $frequency:


            if ($frequency > 2 || ($frequency == 2 && $data['sent']) || in_array($member, $done_members)
                || (!empty($this->_details['members_only']) && !in_array($member, $this->_details['members_only'])))
                continue;


I tested the above code and it fixed the problem (I reproduced the issue as I understood it). Email/alerts were issued after each new topic on watched board when user had the "Straight Away" option selected. Functionality performed as expected when the other one (unread replies only) was selected.

-

It's pretty late, and it took me a bit to familiarize myself with that file's code, so this is kind of rushed. Backup the file before you make the change, but it should be OK afaik. I'll leave it to you to figure out if there are any unforeseen issues or other complications.

Arantor

Have to say at a glance, that looks legit. Needs more testing and I didn't expect it to be a huge piece of work, I'm just super bitter about contributing to SMF 2.1 these days.

TomasM

Hi, Virginiaz, thank you very much for the valuable input!

About a month ago there was a commit that changed the string in question from:

if ($frequency > 2 || (!empty($frequency) && $data['sent']) || in_array($member, $done_members)

to

if (empty($frequency) || $frequency > 2 || $data['sent'] || in_array($member, $done_members)

Could you please recommend how to implement your fix for the new version of this line?

Thank YOU very much!

vii

Quote from: TomasM on August 08, 2018, 10:18:38 PM
Hi, Virginiaz, thank you very much for the valuable input!

About a month ago there was a commit that changed the string in question from:

if ($frequency > 2 || (!empty($frequency) && $data['sent']) || in_array($member, $done_members)

to

if (empty($frequency) || $frequency > 2 || $data['sent'] || in_array($member, $done_members)

Could you please recommend how to implement your fix for the new version of this line?

Thank YOU very much!
If that commit goes into the final project, it means every notification frequency will be subject to the unread replies behavior. In other words, you'll never get more than one notification if you don't go back and read after each one (not sure how the 'digests' options are handled tho). It will perpetuate this same issue and maybe cause others.


-


Just do what I said in my post. Replace the code I mentioned in the first code box with the code in the second box. I explained it thoroughly.

TomasM

Thank you!

Will do, I just wanted to let you know about the new version.

vii

oops, I had misread your post, I interpreted it as how to implement the changes in general, not for the new one.

Ok, I just looked up the commit you referred to, and it appears they were fixing another issue about it apparently not respecting a user's choice to not receive notifications, which they did, but it didn't fix your issue and if anything might have created another one (not entirely sure right now). Here are the new revisions:


            if (empty($frequency) || $frequency > 2 || ($frequency == 2 && $data['sent']) || in_array($member, $done_members)
|| (!empty($this->_details['members_only']) && !in_array($member, $this->_details['members_only'])))
                continue;


That ^ respects the changes made in the aforementioned commit without causing any other issues. Although I want to note that I'm not endorsing that person's changes. I don't know if what they did is correct or makes the notifications behave as expected. I'm just giving you an alternative version of my fix that works with it.

Also apologies if I came off rude before.

Advertisement: