Project

General

Profile

Bug #5738

Sender is platform admin email, instead of teacher email, when a teacher send an announcement to students

Added by Hubert Borderiou over 6 years ago. Updated over 6 years ago.

Status:
Bug resolved
Priority:
Normal
Category:
Announcements
Target version:
Start date:
16/11/2012
Due date:
% Done:

90%

Estimated time:
Spent time:
Complexity:
Normal
SCRUM pts - complexity:
?

Description

Hi
I'm in a course as a teacher
when I send an announcement to students, they receive the mail with sender email is the platform admin's one.
It should be the teacher email as sender.


Files

notification.lib.php (10.2 KB) notification.lib.php Yoselyn Castillo, 28/11/2012 13:40

Related issues

Related to Chamilo LMS - Bug #5445: Announcements are sent from webmasterNew11/09/2012

Actions
Related to Chamilo LMS - Support #4377: configure email senderNeeds more info13/02/2012

Actions

Associated revisions

Revision 4e37bacb (diff)
Added by Hubert Borderiou over 6 years ago

Fix annoucement sender issue - ref #5738

Revision 2f7a3432 (diff)
Added by Hubert Borderiou over 6 years ago

Sender of annoucement is teacher instead of admin - ref #5738

History

#1

Updated by Julio Montoya over 6 years ago

I think we already discussed this issue in another task, a change in the DB is needed it. I think that it was already fixed in 1.10 (not 100% sure)

#2

Updated by Hubert Borderiou over 6 years ago

  • Status changed from New to Needs testing

It's ok if we modified file notification.lib.php
We had line

api_mail_html($name, $user_info['mail'], Security::filter_terms($title), Security::filter_terms($content), $this->admin_name, $this->admin_email);

it's ok if I replace with

api_mail_html($name, $user_info['mail'], Security::filter_terms($title), Security::filter_terms($content), $sender_info['complete_name'], $sender_info['email']);

fix sent
http://code.google.com/p/chamilo/source/detail?r=27315bc3997639fe617b48ffd331077188bd0476&name=1.9.x&repo=classic

#3

Updated by Yoselyn Castillo over 6 years ago

  • Status changed from Needs testing to Assigned
  • Assignee set to Yoselyn Castillo

I will test and i'll give you my feedback

#4

Updated by Julio Montoya over 6 years ago

In some cases the $sender_info array will be a "group data" and not a "user data" and it will only work when sending the message "at once" and not when sending the message in a "batch process".

See the MessageManager::send_message function:


     $group_info = GroupPortalManager::get_group_data($group_id);
                $group_info['topic_id'] = $topic_id;
                $group_info['msg_id']     = $inbox_last_id;

                $user_list  = GroupPortalManager::get_users_by_group($group_id, false, array(),0, 1000);

                //Adding more sens to the message group
                $subject = sprintf(get_lang('ThereIsANewMessageInTheGroupX'), $group_info['name']);                

                $new_user_list = array();           
                foreach($user_list as $user_data) {
                    $new_user_list[] = $user_data['user_id'];
                }
                $group_info = array('group_info'=>$group_info, 'user_info' => $sender_info);
                $notification->save_notification(NOTIFICATION_TYPE_GROUP, $new_user_list, $subject, $content, $group_info); 

I suggest to undo that change, otherwise some bugs will appeared here and there ...

#5

Updated by Yoselyn Castillo over 6 years ago

Thanks i will follow your suggestions

#6

Updated by Hubert Borderiou over 6 years ago

  • Subject changed from Sender is platform admin email, instead of teacher email, when a teacher send an announcement to students to GRB - Sender is platform admin email, instead of teacher email, when a teacher send an announcement to students
  • Priority changed from Normal to High
#7

Updated by Yoselyn Castillo over 6 years ago

It works fine with the change of Hubert when a teacher send an announcement to a group sender is the teacher email, not the platform admin, so in which case the sender could be a "group data"?
Anyway, this file has the platform admin again..

#8

Updated by Julio Montoya over 6 years ago

  • Assignee changed from Julio Montoya to Yoselyn Castillo

When you change your profile notification settings in "Once a day"

Then you execute the cron /var/www/chamilonet/main/cron/notification.php (this script will send all pending messages).

This bug could appear when posting message in "social groups".

In comment # 4 you see that the $notification->save_notification() receives a group_data and not a user_data

#9

Updated by Yoselyn Castillo over 6 years ago

  • Status changed from Assigned to Needs more info
  • Assignee changed from Yoselyn Castillo to Julio Montoya

So, we should let the sender of announcements to be the platform admin isn't it?

#10

Updated by Hubert Borderiou over 6 years ago

So, we should let the sender of announcements to be the platform admin isn't it?

I don't think so.
We should add an exception for the specific use of social group in this specific case.
It is really important that the sender of announcement be the teacher. Announcement is a basic function of the platform. It couldn't be damaged by the specific use of an advanced tool, could it ?

#11

Updated by Julio Montoya over 6 years ago

  • Assignee changed from Julio Montoya to Hubert Borderiou

Hubert Borderiou wrote:

So, we should let the sender of announcements to be the platform admin isn't it?

I don't think so.
We should add an exception for the specific use of social group in this specific case.
It is really important that the sender of announcement be the teacher. Announcement is a basic function of the platform. It couldn't be damaged by the specific use of an advanced tool, could it ?

If we do that (for 1.9.4) the notifications (notification settings in the user profile) will not work correctly.

This is the notification table:

mysql> describe notification;
+--------------+-------------+------+-----+---------+----------------+
| Field        | Type        | Null | Key | Default | Extra          |
+--------------+-------------+------+-----+---------+----------------+
| id           | bigint(20)  | NO   | PRI | NULL    | auto_increment |
| dest_user_id | int(11)     | NO   |     | NULL    |                |
| dest_mail    | char(255)   | YES  |     | NULL    |                |
| title        | char(255)   | YES  |     | NULL    |                |
| content      | char(255)   | YES  |     | NULL    |                |
| send_freq    | smallint(6) | YES  |     | 1       |                |
| created_at   | datetime    | NO   |     | NULL    |                |
| sent_at      | datetime    | YES  | MUL | NULL    |                |
+--------------+-------------+------+-----+---------+----------------+
8 rows in set (0.00 sec)

As you can see there's no sender_id because the notification feature was build à la Redmine

User were confused because they reply to the admin or the "no-reply" email we/I overestimate the capacity of users to understand this.

If you suggest to force to send the sender email it will work only for "send at once" option. The tool then will be broken.

For 1.10 I already add the sender_id and it will work as expected.

Other issue about that change is that, I'm not sure if teachers/users want to expose their personal emails ...

I also remember that I wrote that explanation in another task ??

#12

Updated by Hubert Borderiou over 6 years ago

  • Subject changed from GRB - Sender is platform admin email, instead of teacher email, when a teacher send an announcement to students to Sender is platform admin email, instead of teacher email, when a teacher send an announcement to students
  • Priority changed from High to Normal

Anyway, it is ok for annoucements for the moment in 1.9.4
If the teacher send an annoucement, he will be the sender of the mail.
I don't know about the other issue.
It is ok for me if the other issue is fixed in 1.10.0 ;-)

#14

Updated by Julio Montoya over 6 years ago

  • File test.png added

Mail servers detect this change as phishing, I change the code to add the "reply_to" option instead of sending the email as a user see:

https://code.google.com/p/chamilo/source/detail?r=ead81e11cfe69874b021c72bc0b3111dbeaaf6df&name=1.9.x&repo=classic

#15

Updated by Julio Montoya over 6 years ago

  • File deleted (test.png)
#16

Updated by Yannick Warnier over 6 years ago

Julio Montoya wrote:

Mail servers detect this change as phishing, I change the code to add the "reply_to" option instead of sending the email as a user see:

https://code.google.com/p/chamilo/source/detail?r=ead81e11cfe69874b021c72bc0b3111dbeaaf6df&name=1.9.x&repo=classic

This is the same way as Google does it for gmail when you use an e-mail from another domain (this spanned a large discussion at the time, a few years back). There is a solution to this, which is to add a specific (SPF, I think, or MX anyway) record to your DNS configuration to authorize the Chamilo server to send mail from specific other URLs.

I'm no specialist in the matter. I'm adding Jérôme here. Maybe he can enlighten us with the possibilities we have...

#17

Updated by Yannick Warnier over 6 years ago

  • Target version changed from 1.9.4 to 1.9.6

Moving to 1.9.6 as it's a work in progress...

#19

Updated by Yannick Warnier over 6 years ago

  • Status changed from Needs more info to Needs testing
  • Assignee changed from Hubert Borderiou to Yoselyn Castillo
  • % Done changed from 50 to 90
#20

Updated by Yoselyn Castillo over 6 years ago

I have tested in stable.chamilo.org and now no email has been sent when an announcement have been created, even the checkbox has been marked. Checking...

#21

Updated by Yoselyn Castillo over 6 years ago

  • Status changed from Needs testing to Bug resolved

Well, i have made many tests and it works fine, even if the sender is from a group
Messages were sent successfully when an announcement is created and the sender is always the teacher. I think we can close this task and later.

Also available in: Atom PDF