Skip to content

Commit

Permalink
MDL-78738 core_communication: Set avatar from a stored_file
Browse files Browse the repository at this point in the history 
We should always use a stored_file instance to set the avatar, and never
a data uri. Especially where the datauri is from Moodle itself.

This change requires a number of changes in associated locations.

I am not adding this change to any upgrade notes as this is a
master-only feature.

Note: I have removed some tests and will not consider fixing them
because I will be entirely removing that test file in a subsequent issue
(MDL-77917).
  • Loading branch information
@andrewnicols
andrewnicols committed Aug 1, 2023
1 parent 0a5c1ea commit 8b97071
Show file tree
Hide file tree
Showing 12 changed files with 259 additions and 103 deletions.
42 changes: 21 additions & 21 deletions communication/classes/api.php
View file
Expand Up @@ -222,20 +222,19 @@ public function get_avatar_filerecord(string $filename): stdClass {
* If null is set, then delete the old area file and set the avatarfilename to null.
* This will make sure the plugin api deletes the avatar from the room.
*
* @param string|null $datauri The datauri of the avatar
* @param null|\stored_file $avatar The stored file for the avatar
* @return bool
*/
public function set_avatar_from_datauri_or_filepath(?string $datauri): bool {

public function set_avatar(?\stored_file $avatar): bool {
$currentfilename = $this->communication->get_avatar_filename();
if (empty($datauri) && empty($currentfilename)) {
if ($avatar === null && empty($currentfilename)) {
return false;
}

$currentfilerecord = $this->communication->get_avatar();
if (!empty($datauri) && !empty($currentfilerecord)) {
if ($avatar && !empty($currentfilerecord)) {
$currentfilehash = $currentfilerecord->get_contenthash();
$updatedfilehash = \file_storage::hash_from_string(file_get_contents($datauri));
$updatedfilehash = $avatar->get_contenthash();

// No update required.
if ($currentfilehash === $updatedfilehash) {
Expand All @@ -244,7 +243,6 @@ public function set_avatar_from_datauri_or_filepath(?string $datauri): bool {
}

$context = \context_system::instance();
$filename = null;

$fs = get_file_storage();
$fs->delete_area_files(
Expand All @@ -254,13 +252,16 @@ public function set_avatar_from_datauri_or_filepath(?string $datauri): bool {
$this->communication->get_id()
);

if (!empty($datauri)) {
$filename = "avatar.svg";
$fs->create_file_from_string($this->get_avatar_filerecord($filename), file_get_contents($datauri));
if ($avatar) {
$fs->create_file_from_storedfile(
$this->get_avatar_filerecord($avatar->get_filename()),
$avatar,
);
$this->communication->set_avatar_filename($avatar->get_filename());
} else {
$this->communication->set_avatar_filename(null);
}

$this->communication->set_avatar_filename($filename);

// Indicate that we need to sync the avatar when the update task is run.
$this->communication->set_avatar_synced_flag(false);

Expand Down Expand Up @@ -299,16 +300,15 @@ public function get_provider(): string {
*
* @param string $selectedcommunication The selected communication provider
* @param string $communicationroomname The communication room name
* @param null|\stored_file $avatar The stored file for the avatar
* @param \stdClass|null $instance The actual instance object
* @param string|null $avatarurl The avatar url
*/
public function create_and_configure_room(
string $selectedcommunication,
string $communicationroomname,
?string $avatarurl = null,
?\stored_file $avatar = null,
?\stdClass $instance = null,
): void {

if ($selectedcommunication !== processor::PROVIDER_NONE && $selectedcommunication !== '') {
// Create communication record.
$this->communication = processor::create_instance(
Expand All @@ -325,8 +325,8 @@ public function create_and_configure_room(
}

// Set the avatar.
if (!empty($avatarurl)) {
$this->set_avatar_from_datauri_or_filepath($avatarurl);
if (!empty($avatar)) {
$this->set_avatar($avatar);
}

// Add ad-hoc task to create the provider room.
Expand All @@ -342,13 +342,13 @@ public function create_and_configure_room(
*
* @param string $selectedprovider The selected communication provider
* @param string $communicationroomname The communication room name
* @param null|\stored_file $avatar The stored file for the avatar
* @param \stdClass|null $instance The actual instance object
* @param string|null $avatarurl The avatar url
*/
public function update_room(
string $selectedprovider,
string $communicationroomname,
?string $avatarurl = null,
?\stored_file $avatar = null,
?\stdClass $instance = null,
): void {

Expand All @@ -367,7 +367,7 @@ public function update_room(
}

// Update the avatar.
$imageupdaterequired = $this->set_avatar_from_datauri_or_filepath($avatarurl);
$imageupdaterequired = $this->set_avatar($avatar);

// If the provider is none, we don't need to do anything from room point of view.
if ($this->communication->get_provider() === processor::PROVIDER_NONE) {
Expand All @@ -392,7 +392,7 @@ public function update_room(
}
} else {
// The instance didn't have any communication record, so create one.
$this->create_and_configure_room($selectedprovider, $communicationroomname, $avatarurl, $instance);
$this->create_and_configure_room($selectedprovider, $communicationroomname, $avatar, $instance);
}
}

Expand Down
3 changes: 1 addition & 2 deletions communication/provider/matrix/classes/communication_feature.php
View file
Expand Up @@ -304,7 +304,6 @@ public function delete_chat_room(): bool {
* Update the room avatar when an instance image is added or updated.
*/
public function update_room_avatar(): void {

// Check if we have an avatar that needs to be synced.
if (!$this->communication->is_avatar_synced()) {

Expand All @@ -313,7 +312,7 @@ public function update_room_avatar(): void {

// If avatar is set for the instance, upload to Matrix. Otherwise, leave null for unsetting.
if (!empty($instanceimage)) {
$contenturi = $this->eventmanager->upload_matrix_content($instanceimage->get_content());
$contenturi = $this->eventmanager->upload_matrix_content($instanceimage);
}

$response = $this->eventmanager->request(['url' => $contenturi], [], false)->put(
Expand Down
25 changes: 20 additions & 5 deletions communication/provider/matrix/classes/matrix_events_manager.php
View file
Expand Up @@ -17,6 +17,7 @@
namespace communication_matrix;

use core\http_client;
use stored_file;
use GuzzleHttp\Psr7\Request;

/**
Expand Down Expand Up @@ -223,19 +224,33 @@ public function request(array $jsonarray = [], array $headers = [], bool $httper
/**
* Upload the content in the matrix/synapse server.
*
* @param string $content The content to be uploaded
* @param null|stored_file $file The content to be uploaded
* @return string|false
*/
public function upload_matrix_content(string $content): bool|string {
public function upload_matrix_content(?stored_file $file): bool|string {
$headers = [
'Authorization' => 'Bearer ' . $this->get_token(),
'Content-Type' => 'image/png'
];
$filecontent = null;
$query = [];

if ($file) {
$filecontent = $file->get_content();
$headers['Content-Type'] = $file->get_mimetype();
$query['filename'] = $file->get_filename();
}

$client = new http_client();
$request = new Request('POST', $this->get_upload_content_endpoint(), $headers, $content);
$request = new Request(
'POST',
$this->get_upload_content_endpoint(),
$headers,
$filecontent,
);

$response = $client->send($request);
$response = $client->send($request, [
'query' => $query,
]);
$response = json_decode($response->getBody());
if ($response) {
return $response->content_uri;
Expand Down
119 changes: 84 additions & 35 deletions communication/provider/matrix/tests/communication_feature_test.php
View file
Expand Up @@ -16,8 +16,10 @@

namespace communication_matrix;

use core_communication\api;
use core_communication\processor;
use core_communication\communication_test_helper_trait;
use stored_file;

defined('MOODLE_INTERNAL') || die();

Expand Down Expand Up @@ -132,39 +134,96 @@ public function test_delete_chat_room(): void {
* Test update room avatar.
*
* @covers ::update_room_avatar
* @dataProvider avatar_provider
*/
public function test_update_room_avatar(): void {
global $CFG;
$course = $this->get_course('Sampleroom', 'none');
public function test_update_room_avatar(
?string $before,
?string $after,
): void {
$this->setAdminUser();

// Create a new draft file.
$logo = $this->create_communication_file('moodle_logo.jpg', 'logo.jpg');
$circle = $this->create_communication_file('circle.png', 'circle.png');

if ($before === 'logo') {
$before = $logo;
} else if ($before === 'circle') {
$before = $circle;
}

if ($after === 'logo') {
$after = $logo;
} else if ($after === 'circle') {
$after = $circle;
}

$communication = $this->create_matrix_room(
component: 'communication_matrix',
itemtype: 'example_room',
itemid: 1,
roomname: 'Example room name',
roomavatar: $before,
);

// Sample data.
$communicationroomname = 'Sampleroom';
$selectedcommunication = 'communication_matrix';
$avatarurl = $CFG->dirroot . '/communication/tests/fixtures/moodle_logo.jpg';
// Confirm that the avatar was set remotely.
$remoteroom = $this->backoffice_get_room();

if ($before) {
$this->assertStringEndsWith($before->get_filename(), $remoteroom->avatar);
$avatarcontent = download_file_content($remoteroom->avatar);
$this->assertEquals($before->get_content(), $avatarcontent);
} else {
$this->assertEmpty($remoteroom->avatar);
}

// Reload the API instance as the information stored has changed.
$communication = \core_communication\api::load_by_instance(
'core_course',
'coursecommunication',
$course->id
);
$communication->create_and_configure_room(
$selectedcommunication,
$communicationroomname,
$avatarurl
component: 'communication_matrix',
instancetype: 'example_room',
instanceid: 1,
);

$communicationprocessor = processor::load_by_instance(
'core_course',
'coursecommunication',
$course->id
// Update the avatar with the 'after' avatar.
$communication->update_room(
'communication_matrix',
'Example room name',
avatar: $after,
);
$communicationprocessor->get_room_provider()->create_chat_room();

$matrixrooms = new matrix_rooms($communicationprocessor->get_id());
$this->run_all_adhoc_tasks();

// Confirm that the avatar was updated remotely.
$remoteroom = $this->backoffice_get_room();

if ($after) {
$this->assertStringEndsWith($after->get_filename(), $remoteroom->avatar);
$avatarcontent = download_file_content($remoteroom->avatar);
$this->assertEquals($after->get_content(), $avatarcontent);
} else {
$this->assertEmpty($remoteroom->avatar);
}
}

// Add api call to get room data and test against set data.
$matrixroomdata = $this->get_matrix_room_data($matrixrooms->get_matrix_room_id());
$this->assertNotEmpty($matrixroomdata->avatar);
/**
* Tests for setting and updating the room avatar.
*
* @return array
*/
public function avatar_provider(): array {
return [
'Empty to avatar' => [
null,
'circle',
],
'Avatar to empty' => [
'circle',
null,
],
'Avatar to new avatar' => [
'circle',
'logo',
],
];
}

/**
Expand All @@ -173,13 +232,11 @@ public function test_update_room_avatar(): void {
* @covers ::get_chat_room_url
*/
public function test_get_chat_room_url(): void {
global $CFG;
$course = $this->get_course('Sampleroom', 'none');

// Sample data.
$communicationroomname = 'Sampleroom';
$selectedcommunication = 'communication_matrix';
$avatarurl = $CFG->dirroot . '/communication/tests/fixtures/moodle_logo.jpg';

$communication = \core_communication\api::load_by_instance(
'core_course',
Expand All @@ -190,7 +247,6 @@ public function test_get_chat_room_url(): void {
$communication->create_and_configure_room(
$selectedcommunication,
$communicationroomname,
$avatarurl
);

$communicationprocessor = processor::load_by_instance(
Expand All @@ -217,14 +273,12 @@ public function test_get_chat_room_url(): void {
* @covers ::add_registered_matrix_user_to_room
*/
public function test_create_members(): void {
global $CFG;
$course = $this->get_course('Sampleroom', 'none');
$userid = $this->get_user()->id;

// Sample data.
$communicationroomname = 'Sampleroom';
$selectedcommunication = 'communication_matrix';
$avatarurl = $CFG->dirroot . '/communication/tests/fixtures/moodle_logo.jpg';

$communication = \core_communication\api::load_by_instance(
'core_course',
Expand All @@ -235,7 +289,6 @@ public function test_create_members(): void {
$communication->create_and_configure_room(
$selectedcommunication,
$communicationroomname,
$avatarurl
);
$communication->add_members_to_room([$userid]);

Expand Down Expand Up @@ -274,14 +327,12 @@ public function test_create_members(): void {
* @covers ::check_room_membership
*/
public function test_add_and_remove_members_from_room(): void {
global $CFG;
$course = $this->get_course('Sampleroom', 'none');
$userid = $this->get_user()->id;

// Sample data.
$communicationroomname = 'Sampleroom';
$selectedcommunication = 'communication_matrix';
$avatarurl = $CFG->dirroot . '/communication/tests/fixtures/moodle_logo.jpg';

$communication = \core_communication\api::load_by_instance(
'core_course',
Expand All @@ -292,7 +343,6 @@ public function test_add_and_remove_members_from_room(): void {
$communication->create_and_configure_room(
$selectedcommunication,
$communicationroomname,
$avatarurl
);
$communication->add_members_to_room([$userid]);

Expand Down Expand Up @@ -343,5 +393,4 @@ public function test_save_form_data(): void {
$matrixroomdata = new matrix_rooms($communicationprocessor->get_id());
$this->assertEquals('Sampletopicupdated', $matrixroomdata->get_matrix_room_topic());
}

}

0 comments on commit 8b97071

Please sign in to comment.