Moodle

Community block

Details

  • Type: Sub-task Sub-task
  • Status: Closed Closed
  • Priority: Critical Critical
  • Resolution: Fixed
  • Affects Version/s: 2.0
  • Fix Version/s: 2.0
  • Component/s: Hub
  • Labels:
    None
  • Difficulty:
    Moderate
  • Affected Branches:
    MOODLE_20_STABLE
  • Fixed Branches:
    MOODLE_20_STABLE

Description

Functional specification: http://docs.moodle.org/en/Community_hub#Community_membership

Issue Links

This issue has been marked as being related by:
MDL-22829 General community block improvement Minor Closed

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Petr Škoda (skodak) added a comment -

HEY!
1/ stop committing CRLF
2/ you can not just drop YUI modules into lib, that is going to cause horrible problems in the future, we MUST version them properly is an extra directory and also we MUST configure PHP loader

Please delete the /lib/gallery/ immediately and prepare a proper patch that adds complete YUI module support, or wait till I do that after enrol+editors.

thanks!

Show
Petr Škoda (skodak) added a comment - HEY! 1/ stop committing CRLF 2/ you can not just drop YUI modules into lib, that is going to cause horrible problems in the future, we MUST version them properly is an extra directory and also we MUST configure PHP loader Please delete the /lib/gallery/ immediately and prepare a proper patch that adds complete YUI module support, or wait till I do that after enrol+editors. thanks!
Hide
Permalink
Petr Škoda (skodak) added a comment -

OH! it is absolutely unacceptable to use $this->page->requires->css('/lib/gallery/assets/skins/sam/gallery-lightbox-skin.css');, please never ever do this again, this is only for core stuff, your code must not use it.

Which part of the following did you not understand?
/**

  • Ensure that the specified CSS file is linked to from this page.
    *
  • Because stylesheet links must go in the <head> part of the HTML, you must call
  • this function before {@link get_head_code()} is called. That normally means before
  • the call to print_header. If you call it when it is too late, an exception
  • will be thrown.
    *
  • Even if a particular style sheet is requested more than once, it will only
  • be linked to once.
    *
  • Please note sue of this feature is strongly discouraged,
  • it is suitable only for places where CSS is submitted directly by teachers.
  • (Students must not be allowed to submit any external CSS because it may
  • contain embedded javascript!). Example of correct use is mod/data.
    *
  • @param string $stylesheet The path to the .css file, relative to $CFG->wwwroot.
  • For example:
  • $PAGE->requires->css('mod/data/css.php?d='.$data->id);
    */
    public function css($stylesheet) {
Show
Petr Škoda (skodak) added a comment - OH! it is absolutely unacceptable to use $this->page->requires->css('/lib/gallery/assets/skins/sam/gallery-lightbox-skin.css');, please never ever do this again, this is only for core stuff, your code must not use it. Which part of the following did you not understand? /**
  • Ensure that the specified CSS file is linked to from this page. *
  • Because stylesheet links must go in the <head> part of the HTML, you must call
  • this function before {@link get_head_code()} is called. That normally means before
  • the call to print_header. If you call it when it is too late, an exception
  • will be thrown. *
  • Even if a particular style sheet is requested more than once, it will only
  • be linked to once. *
  • Please note sue of this feature is strongly discouraged,
  • it is suitable only for places where CSS is submitted directly by teachers.
  • (Students must not be allowed to submit any external CSS because it may
  • contain embedded javascript!). Example of correct use is mod/data. *
  • @param string $stylesheet The path to the .css file, relative to $CFG->wwwroot.
  • For example:
  • $PAGE->requires->css('mod/data/css.php?d='.$data->id); */ public function css($stylesheet) {
Hide
Permalink
Jerome Mouneyrac added a comment -

Ping, Sam for you

Show
Jerome Mouneyrac added a comment - Ping, Sam for you
Hide
Permalink
Petr Škoda (skodak) added a comment -

and when you are at it, please:
a/ stop adding rubbish strings into moodle.php (publicdirectory0, unknownprivacy) - have to be in hub lang file
b/ use proper prefixes - all functions and classes in comunity block must be prefixed with 'block_community' - community class is very wrong
c/ all hub constants MUST have prefix - stop polluting global scope immediately (lib/hublib.php is really not good coding style)
d/ add proper access control
e/ main block class must not include locallib on each page, do it the other way around!
f/ etc.

Show
Petr Škoda (skodak) added a comment - and when you are at it, please: a/ stop adding rubbish strings into moodle.php (publicdirectory0, unknownprivacy) - have to be in hub lang file b/ use proper prefixes - all functions and classes in comunity block must be prefixed with 'block_community' - community class is very wrong c/ all hub constants MUST have prefix - stop polluting global scope immediately (lib/hublib.php is really not good coding style) d/ add proper access control e/ main block class must not include locallib on each page, do it the other way around! f/ etc.
Hide
Permalink
Jerome Mouneyrac added a comment -

1- I tried to changed CRLF in Netbeans since last time you alerted me in the first hub review. I did a change on Netbeans but apparently it doesn't work. I have to apparently use something else that Netbeans...
2- All YUI stuff are new to me. The integration that was suggested (by someone who a bit of JS) seemed good to me. But, even if a doc existed, people can do mistake on using new design.
3- The publicdirectory0 string and company comes from 1.9, not me. Because I was in a rush to deliver hub for Preview, it has been ordered to be to make the all thing work before to make it perfect. It was plan to back now to fix/improve every part. I put the unknownprivacy with the other, so I would be fixing all strings of these form element at the same time.
4- I would fix the missing prefix.
6- Just to let you know how I work, when I don't know something, I look at the existing code. I could take the wrong example, nothing can't be perfect.
7- add proper access control: sorry I don't understand what you mean
8- main block class must not include locallib on each page, do it the other way around
9- "community class is very wrong", "lib/hublib.php is really not good coding style", "f/ etc" : I will go back today to try to improve them. As I said we were in the rush, but even though, I tried to write them well, I don't think they are that crap!
10- I appreciate that you notice all these stuff otherwise they could not be fixed and I can not improve my coding, but please we work as a team, try to encourage me with your knowledge, not discourage me.

Show
Jerome Mouneyrac added a comment - 1- I tried to changed CRLF in Netbeans since last time you alerted me in the first hub review. I did a change on Netbeans but apparently it doesn't work. I have to apparently use something else that Netbeans... 2- All YUI stuff are new to me. The integration that was suggested (by someone who a bit of JS) seemed good to me. But, even if a doc existed, people can do mistake on using new design. 3- The publicdirectory0 string and company comes from 1.9, not me. Because I was in a rush to deliver hub for Preview, it has been ordered to be to make the all thing work before to make it perfect. It was plan to back now to fix/improve every part. I put the unknownprivacy with the other, so I would be fixing all strings of these form element at the same time. 4- I would fix the missing prefix. 6- Just to let you know how I work, when I don't know something, I look at the existing code. I could take the wrong example, nothing can't be perfect. 7- add proper access control: sorry I don't understand what you mean 8- main block class must not include locallib on each page, do it the other way around 9- "community class is very wrong", "lib/hublib.php is really not good coding style", "f/ etc" : I will go back today to try to improve them. As I said we were in the rush, but even though, I tried to write them well, I don't think they are that crap! 10- I appreciate that you notice all these stuff otherwise they could not be fixed and I can not improve my coding, but please we work as a team, try to encourage me with your knowledge, not discourage me.
Hide
Permalink
Martin Dougiamas added a comment -

Petr, thanks for picking up all those. Valid points, especially the naming of classes and defines. Appreciate the frustration, but please understand that at least some of the rest is because of quite new conventions and not intentional.

1) Exactly how do you recommend new YUI modules be added and versioned etc? We need a HOW TO there. http://docs.moodle.org/en/Development:YUI

2) What is the correct way to include CSS for a case like this, if that function can not be used?

Show
Martin Dougiamas added a comment - Petr, thanks for picking up all those. Valid points, especially the naming of classes and defines. Appreciate the frustration, but please understand that at least some of the rest is because of quite new conventions and not intentional. 1) Exactly how do you recommend new YUI modules be added and versioned etc? We need a HOW TO there. http://docs.moodle.org/en/Development:YUI 2) What is the correct way to include CSS for a case like this, if that function can not be used?
Hide
Permalink
Jerome Mouneyrac added a comment -

Petr,
about CRFL, CR, FL can you tell how you see them?
Why is it a problem for you?
Which files are concerned?
Even on linux I could not find a pb with my committed files...
I also tried Textwrangler that have an option to translate line breaks, but can't find a way to check that modif works...

Show
Jerome Mouneyrac added a comment - Petr, about CRFL, CR, FL can you tell how you see them? Why is it a problem for you? Which files are concerned? Even on linux I could not find a pb with my committed files... I also tried Textwrangler that have an option to translate line breaks, but can't find a way to check that modif works...
Hide
Permalink
Sam Hemelryk added a comment -

Hi Petr,
I've had a look at JavaScript side of things for this and have made the following changes:

  1. Created an image_gallery component in lib/outputcomponents.php that handles all output and initialisation.
  2. JS definitions now handled by static method of image_gallery and proper module definitions used.
  3. Moved lib/gallery/* to lib/gallery/2010060100/* as some form of versioning not sure if this is what you mean by versioning however.
  4. Added readme_moodle.txt and license.txt
  5. Added entry to thirdpartylibs.xml

I wasn't too sure what you meant about versioning the imported libs could you please elaborate on this?

Cheers
Sam

Show
Sam Hemelryk added a comment - Hi Petr, I've had a look at JavaScript side of things for this and have made the following changes:
  1. Created an image_gallery component in lib/outputcomponents.php that handles all output and initialisation.
  2. JS definitions now handled by static method of image_gallery and proper module definitions used.
  3. Moved lib/gallery/* to lib/gallery/2010060100/* as some form of versioning not sure if this is what you mean by versioning however.
  4. Added readme_moodle.txt and license.txt
  5. Added entry to thirdpartylibs.xml
I wasn't too sure what you meant about versioning the imported libs could you please elaborate on this? Cheers Sam
Hide
Permalink
Petr Škoda (skodak) added a comment -

1/ extra YUI modules - Sam is working on that, we just discussed where to find some info related to recommended way to do that

2/ CSS - the new themes framework loads all CSS automatically, the only exception is CSS submitted by teachers (such as in mod/data); if you need CSS in Javascript code it has to be loaded through the YUI loader so that it gets injected before our CSS on the fly when necessary.

So the problems with the way it was added are that it can not be overridden by themes, it would break really badly during the next upgrade and you would be able to use the >requires>css() only before you start page output which would disqualify it from most of the current code in blocks, mforms, etc.

3/ access control? grep for "capability" in blocks/community/communitycourse.php and related files

Thanks everybody and sorry for the harsh words
Petr

Show
Petr Škoda (skodak) added a comment - 1/ extra YUI modules - Sam is working on that, we just discussed where to find some info related to recommended way to do that 2/ CSS - the new themes framework loads all CSS automatically, the only exception is CSS submitted by teachers (such as in mod/data); if you need CSS in Javascript code it has to be loaded through the YUI loader so that it gets injected before our CSS on the fly when necessary. So the problems with the way it was added are that it can not be overridden by themes, it would break really badly during the next upgrade and you would be able to use the >requires>css() only before you start page output which would disqualify it from most of the current code in blocks, mforms, etc. 3/ access control? grep for "capability" in blocks/community/communitycourse.php and related files Thanks everybody and sorry for the harsh words Petr
Hide
Permalink
Sam Hemelryk added a comment -

Hi all,

I've finally sorted a solution to loading the YUI modules from the gallery stored locally within Moodle.
First up how it should all work:

  • Gallery modules are stored in lib/yui/gallery/modulename/version/*
  • Moodle overrides the definition of the loaders gallery group and points it back to Moodle.
  • The group is anonymous so modules don't need declarations however they are expected to define their own needs within the primary module.
  • Requests get made back to yui_combo or for the file directly just like with all other modules.
  • Developers can use gallery modules as follow:
    • $PAGE->requires->js_gallery_module('gallery-lightbox', '2010.04.08-12-35', 'Y.Lightbox.init');
    • $PAGE->requires->js_gallery_module(array('gallery-lightbox','gallery-lightbox-skin'), '2010.04.08-12-35', 'Y.Lightbox.init');

What can't happen at the moment: YUI is still settling on how the gallery CSS should be loaded so presently it doesn't load CSS from the gallery at all!!!
Because of this the checks that would normally allow selection of local or external are being overridden. The problem could be solved by overriding the gallery group for external requests and manually correcting them through the config function however I think that could serious breakage if they ever do implement a better solution. For the time being we always serve locally.

Petr could you please have a look at the patch for me and let me know what you think?

Cheers
Sam

Show
Sam Hemelryk added a comment - Hi all, I've finally sorted a solution to loading the YUI modules from the gallery stored locally within Moodle. First up how it should all work:
  • Gallery modules are stored in lib/yui/gallery/modulename/version/*
  • Moodle overrides the definition of the loaders gallery group and points it back to Moodle.
  • The group is anonymous so modules don't need declarations however they are expected to define their own needs within the primary module.
  • Requests get made back to yui_combo or for the file directly just like with all other modules.
  • Developers can use gallery modules as follow:
    • $PAGE->requires->js_gallery_module('gallery-lightbox', '2010.04.08-12-35', 'Y.Lightbox.init');
    • $PAGE->requires->js_gallery_module(array('gallery-lightbox','gallery-lightbox-skin'), '2010.04.08-12-35', 'Y.Lightbox.init');
What can't happen at the moment: YUI is still settling on how the gallery CSS should be loaded so presently it doesn't load CSS from the gallery at all!!! Because of this the checks that would normally allow selection of local or external are being overridden. The problem could be solved by overriding the gallery group for external requests and manually correcting them through the config function however I think that could serious breakage if they ever do implement a better solution. For the time being we always serve locally. Petr could you please have a look at the patch for me and let me know what you think? Cheers Sam
Hide
Permalink
Sam Hemelryk added a comment -

Re-attached patch, I had missed the correction of images from within the gallery modules CSS.

Show
Sam Hemelryk added a comment - Re-attached patch, I had missed the correction of images from within the gallery modules CSS.
Hide
Permalink
Petr Škoda (skodak) added a comment -

looks ok, please commit

Show
Petr Škoda (skodak) added a comment - looks ok, please commit
Hide
Permalink
Jerome Mouneyrac added a comment -

Ok I think I corrected all Petr review points:

  • I'm now using SmartCVS for committing and adding my files in CVS, we tested a committed file on Sam's Linux machine apparently I don't add any CRLF
  • I renamed all my functions with block_community_ into the community block class manager (renamed the class too)
  • I prefixed all the define in hublib.php
  • I removed the outdated 1.9 registration strings from moodle.php
  • I added access control to the community block (moodle/community:add and moodle/community:download). Someone with moodle/community:add can search and add a community course to his block. If he has the download capability, he can also download courses.

Thank you.

Show
Jerome Mouneyrac added a comment - Ok I think I corrected all Petr review points:
  • I'm now using SmartCVS for committing and adding my files in CVS, we tested a committed file on Sam's Linux machine apparently I don't add any CRLF
  • I renamed all my functions with block_community_ into the community block class manager (renamed the class too)
  • I prefixed all the define in hublib.php
  • I removed the outdated 1.9 registration strings from moodle.php
  • I added access control to the community block (moodle/community:add and moodle/community:download). Someone with moodle/community:add can search and add a community course to his block. If he has the download capability, he can also download courses.
Thank you.

People

Vote (0)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved:
Atlassian JIRA (v4.4.3#663-r165197) | Report a problem
Powered by a free Atlassian JIRA open source license for Moodle. Try JIRA - bug tracking software for your team.