learnweb / moodle-block_qrcode Goto Github PK

Currently, only admins can choose if a logo is embedded in the QR code or not. You could add a feature, so that teachers can choose between these options too.

Hi learnweb-team,

I'd like to propose a second link/button (e.g. next to the download button)
that opens a new window/tab in which the respective QR-code ist displayed
at maximum height.

The goal of this improvement is

• to provide teachers with an easy way to show the QR code in class
• as big as possible
• without causing theme related issues (e.g. maximise via JavaScript)

Cheers,
Christian

bacon/bacon-qr-code fixed a problem with high error correcting QR codes in version 1.0.3. This is transitively required by endroid/QrCode. We should update all dependencies to use this fix.

Hi,

Is it possible to have a different logo per course? Our Moodle Platform is multi-tenanted and we would like a different tenant logo for their respective courses.

Appreciate the assist.

Cheers,
Ray

The Travis badge in README.md points to Travis configured with the old repository, which is why the badge says "failing" – please update! :)

IMO all screenshots should be changed such that they show the default logo (except the admin settings one which should intentionally display a non-standard logo).
Please update the README accordingly and let me now, so that I can change the screenshots in Moodle's plugin directory as well.

Hi,

I've recently installed the QR block (in Moodle version 3.9.7) but when the "Use logo" option is selected in the configure block screen, and a logo uploaded, the QR code fails to appear on screen. The error message states that the logo is trying to pull from an invalid path, but I asked Moodle support to look into it and they couldn't find any issue with the logo's file path. They suggested it could be an issue with the block itself.

Thanks.

This is the least important, but should be done before (public) release:

Please write a composer.json that declares your dependency on QrCode.

https://moodle.org/plugins/pluginversion.php?id=15124&smurf=html (private until positive review) lists a couple of minor issues. Please fix as many as possible.

The lines within then / else are mostly identical. They could be simplified like this:

if ($download) { require_capability(...); }
$outputimg = new...

If no custom logo is selected, a default logo should be used. I suggest going for the Moodle logo. It is always installed with Moodle and can be read from

$CFG->wwwroot/pix/moodlelogo.png or $CFG->wwwroot/pix/moodlelogo.svg, respectively.

The default logo does not look so good. We should consider a modification (maybe something simpler, just the M, with an added white border).

This does not work properly, because we use local caches (i.e. local to each webserver) and eight webservers. Therefore, the statement above will only delete cached images from one server, but not from the others. (Same for PNG!)

(Side note: I consider a combination of file name pattern matching and unlink harmful (who knows what the patterns will match and which files will be (accidentally?!) deleted.)

Effectively we need another form of cache invalidation that does not involve deleting. For example, append the content hash of the logo file to the cached filenames. As a result, when the logo will change, the expected cached filenames will be different so new files are created.

A cron job could take care of deleting old files. Please look for inspiration in other core plugins that use localcache to see how they clean up their old files.

There are variables that are directly reflected in filenames of a Moodle installation:
$file = $CFG->localcachedir . '/block_qrcode/course-' . $courseid . '-' . $size . '-0';

You need to make sure that those variables (here: $courseid, $size) can never contain anything that is potentially dangerous. In this case this is simple by casting them to integer. Then, at worst, they are 0, but don't do anything wrong.

It's best to do this at the beginning, i.e. $this->size = $size; becomes $this->size = (int)$size;. courseid analogous.

It would be great to be able to trace how many people enter a course by following the QR code. With UTM Google Analytics has established a quasi-standard that is used throughout the web.

We should add such a parameter to the encoded URL so that it is present when someone opens a course by following a QR code. We would subsequently be able to measure this in Piwik or server logs. I think utm_source is suited best, but please check if one of the others has an even better fit.

Edited as per #25 (comment):

When settings are changed, particularly the logo upload, add two previews of what QR codes will look like. This allows admins to check whether the codes will look as intended. In particular, it may not be obvious from the start that there is a default logo, and that uploading just a single format type will not have an impact on all QR codes! A preview would make this more apparent.

--------- original text -------------

Hello,

Thanks for your work on this plugin.

It would be nice to add the QR code image reloading on format selecting. It is not clear for user, how it will looks like, when he/she is trying to download the png file. The image on png file could be different, because there is the possibility in the plugin settings to use different image for each format.

Best regards,
Olena Tatarintseva

Apparently the context ID is only used in

If that is correct, please do not pass the context IDs as GET parameters to download.php, but initialise it there.

-- actually, come to think about it: This is just for retrieving the logo, right? Then this shouldn't be a parameter or function argument at all. The context ID should just be obtained right where you need it, as you do in adminsettings.php.