Moodle

Oops, issue description doesn't support text formatting.

license table:

we already a proposal for file metadata table, I personally do not like the idea of adding this info directly into files table because soon you realise you need more of these fields such as the original author who can not be identified by user id

The problem with the proposed metadata table is that it is general == can not be normalised and linked to other tables. Also the file api operations would have to be updated to copy over the metadata during each file operation which could make it much slower.

In any case it is not too late to modify current table, implement proposed metadata table or come with yet another solution. I personally think we should make some proposal with full design description before we start with implementation changes.

Petr

One thing we know, it's crucial to store the original author and license for each imported file if we know these details, so that we can use the files legally inside Moodle. For example this affects how we display the file (eg adding watermarks) but another use case is to only show creative commons content when browsing files from other users in the file picker.

As Petr said there are three options:

1) In files table
2) In files_metadata table, joined to files somehow
3) In generic metadata table like config table

We probably need more explicit use cases to help decide.

Thanks Martin and Petr - I have emailed and asked one of the folks at CreativeCommons.org to follow this issue and perhaps provide some feedback about use cases. Peace - Anthony

Hi all

Since moodle 2 beta is coming, we probably should get this issue fixed.

I talked with Martin the other day, we have a proposal to add author/source/license fields to file table, because these three are always needed.
Author field:
If author is not provided, the author of the file will be decided by user_id

Source:
When filepicker import a file from external site, we should store the external link or other key information to identify the original source

License:
Will be stored as a short name, the content of license will be stored in language file. If license type is not provided, we should use a very strict default type, like all right reserved.

Things need to be changed:
All functions related to insert new file records, including create_file_from_pathname, create_file_from_string, create_file_from_storedfile, create_file_from_url.

get_file_info function will return an array including author/license/source.

get_file_info* functions should take license/author/source arguments to get a specific file.

Petr, what do you think about it? The only downside of file_metadata table is it maybe bad for performance, we need a decision to fix this issue.

Thank you.

Regards,
Dongsheng Cai

I think this is perfect. Petr, if you can see any real problems with this let Dongsheng know, otherwise I think we should go ahead and put it in. Time is short!!!

yes, the only potential problem I can see is the handling of files in draft areas and the actual structure of metadata table.

The options for metadata table are:
1/ one huge table with very many columns (id, author, license, ext, link....) - fast, less flexible
2/ the general key=>value table - slow, anything can be stored there

The performance problem is that the key=>value metadata table is not normalised - you have to fetch N records for just 1 file - so normal joins can not be done and eventually you might end up with extremely low performance both when reading and especially writing.

Actually what Dongsheng is about to do is not create a new metadata table, but add those three important fields to the file table

3/ put it directly into the table - sorry, that is the simplest way for now
+1

please post a patch here before commit

I wonder why I wrote "please post a patch here before commit" above Anyway here is a review:

–

1/ It is strongly discouraged to use functions from libraries in main db/upgrade.php, the reason is that the library may require some db structure that is not yet upgraded - we were bitten by this many times before in roles code, so please remove the following and replace it with hardcoded db inserts:
require_once($CFG->libdir . '/licenselib.php');
license_manager::install_licenses();

Please note the plugin upgrades are something different, there we should always use the latest API calls.

—

2/ Hmm, how are we going to deal with translation of license names?

—

3/ why is the enabled flag in the license table? when and how is it going to be used?
In general if some setting is used more often we store it in the config table and access through the CFG because it is one less DB query.

—

4/ grrrr - the new setting adds one more db query to admin tree loading - please please stop adding more requests there, we do need to lower the number there significantly - simply add new config class which loads the options only when necessary and also requires the lib only when really necessary.

—

5/ I guess you have forgotten to install the licenses in fresh new installs, right? (db/install.php)

–

6/ I do not think it is good idea to add the default license when adding new file - that is really asking for legal trouble, better to know we actually do not know the license

if ($file_record->license === '') {
            $file_record->license = $CFG->sitedefaultlicense;
        }

–

7/ please, stop doing this in all new code:

if ($license_id = $DB->insert_record('license', $license)) {

–

8/ the new standard for empty result from license_manager::get() should be array() instead of null, the same way as $DB->get_records()
license_manager::get() returns NULL, object or array of objects - that is weird, it should return only array

please do not invent those superflexible function parameters again - create two separate methods with different parameters instead if really necessary

–

9/ I absolutely do not understand the hardcoded list of license in the licenseib.php.

• How can I add my own licenses.
• Why does it link only english version
• How are we going to upgrade this list?
• What does the "enable" actually mean - previous existing files in moodle are made inancessible? Is it only for external systems?

–

10/ how does somebody specify the license when uploading new file from local PC?

–

11/

Sorry! I missed that message about posting patch, I will fix the problem as soon as possbile.

> 2/ Hmm, how are we going to deal with translation of license names?

The full name should go to language file, for license description, I am think we add a link after file name, the link will lead to the license homepage (creative common already had mulit-language editions), if we store license description in language file, we will have more problems

> 3/ why is the enabled flag in the license table? when and how is it going to be used?
enabled flag means if this license will be displayed in list, if enabled = 0, it won't show in filepicker.
Do you think the licenses should work like editors? I saw you created texteditors variable under CFG?

> 6/
yes, I will create unknown type license

> 9/ I absolutely do not understand the hardcoded list of license in the licenseib.php.

> 10/
Use filepicker, when you trying to upload a file, filepicker will ask license from you.

Just about the list of licenses ... there are only a small list of content licenses used in the real world, and we also want to have some compatibility across sites for hubs etc, that's why we have a hard-coded list using language packs. Licenses are legal things, and are generally the result of a lot of discussion and documentation.

We certainly don't want to encourage people to make up new content licenses whenever they want, but if enough people vote for particular new licenses we can add them for everyone.

We definitely do need a default license and the admin should choose it. I'm not sure if "Unknown license" is useful ... without info the content should effectively be treated as if it has no license at all, so let's just call it "All rights reserved".

Martin

How do you think of the license content?
In most cases, we don't display the license content for user in moodle, we usually have a link after filename, should we link to a moodle page to display content or to the original license home page?

Hi, Petr

I am going to create $CFG->licenses, when user made changes in license mangement page, the active license types will stored in this variable as "cc,cc-nc,cc-sa", then we can save one database query?

Do you agree?

Hi, Petr,
I committed a few new code to fix the problem you mentioned:
1. Use hardcoded db insert_record in db/upgrade.php
2. added the missing language file
3. store active licenses under $CFG, it saves one db query
4. added unknown license type
5. I removed license_manage::get() function, now we have: license_manager::get_license_by_shortname, it returns null or a license record, license_manager::get_licenses, it returns an array.

I noticed that you added code to install licenses on fresh installation, thanks, do we need to use hardcoded db inserts in this script as well?

1/ I did not mean to duplicate the $license->enabled = 1 with $CFG->licenses, that does not make much sense to me - one of them is enough imho.

2/ I had to do some changes in file_storage class - I spent a lot of time designing it so that it does not depend on any CFG, the setting of default site license should be really done elsewhere, maybe probably during disaply time - null in files table, then use the default; also I removed some unnecessary cleanup because it has to be done before output anyway

3/ why is the "Default site license" setting not on the "Manage licenses" page.

4/ what happens when you disable the default site license? I got invalid value in Default site license which seem like a potential problem for admins.

5/ which license should be used during upgrade? the default is not selected yet...

I removed the show/hide icon of default license from the list, added a lock icon next to default license, and moved the default license select element to plugin->licenses.

> I did not mean to duplicate the $license->enabled = 1 with $CFG->licenses, that does not make much sense to me - one of them is enough imho.

If I drop the enabled field, should I start a new upgrade code section to drop it? Or do I need to change the original creating table code as well?

setting $CFG->sitedefaultlicense = 'allrightsreserved'; before opening DB connection is wrong, removing it from setup.php - all defaults must be done through the settings tree, we add only defaults for things that must always exist because they are used in system libraries

This seems resolved now.