Moodle in English: Please check first time development with DB and Forms

Please check first time development with DB and Forms

by Gareth J Barnard - Friday, 3 February 2012, 5:24 AM

Hi,

I have created a new version of my Collapsed Topics format (for Moodle 2.2) which uses the database and a form - please see http://moodle.org/mod/forum/discuss.php?d=195292.

I would be really grateful if a 'Guru' could check over the DB and Form code to see that everything is ok. I'm not sure I have got the permissions right so that only users who can edit a course can make a change.

Thanks,

Gareth

Average of ratings: -

Re: Please check first time development with DB and Forms

by Dan Poltawski - Sunday, 5 February 2012, 7:08 PM

Hi Gareth,

Perhaps you could help us by pointing us where specifically you want us to look at it. Its difficult to know where to look at not knowing the collapsed topics format before hand. Do you have a git repository or something with the recent changes?

thanks,

Dan

Average of ratings: -

Re: Please check first time development with DB and Forms

by Gareth J Barnard - Wednesday, 8 February 2012, 4:23 AM

Dear Dan,

I'm really sorry, the link to the course format post contains a zip file as an attachment (now Beta 3 at the bottom of the post) - I did not want to double post the code as that is against Moodle forum's rules. I do have a Git repository but did not want to push development code that was not working to it.

Please would you kindly examine the bottom of lib.php, the files in the db folder and set_layout.php with set_layout_form.php to see if my use of the Moodle DB and Forms is ok along with the correct use of the $PAGE global (in set_layout.php) in only allowing users who can edit a course to do so.

There are not many files in the zip file and installation is simply a matter of copying the 'topcoll' folder into '/course/formats' and going to 'Notifications' as any other plugin.

Many thanks,

Gareth

Average of ratings: -

Re: Please check first time development with DB and Forms

by Andrew Lyons - Wednesday, 8 February 2012, 5:47 PM

Hi Gareth,

When I'm working on these kinds of things, I tend to push a branch with my current state-of-affairs for others to see. When the feature is ready, I then push an updated branch with the changes squashed together.

One of the many wonderful things with git is its ability to rewrite your history. You can (relatively) easily rebase a set of commits, and squash them together into one commit, or break them apart into more than one if that's appropriate (you can do much more than that of course).

Andrew

Average of ratings: -

Re: Please check first time development with DB and Forms

by Andrew Lyons - Wednesday, 8 February 2012, 6:08 PM

Just having a quick look over your code now.

lib.php

In terms of the database code, this looks fine
I personally wouldn't bother with the error - if someone hasn't visited the notifications screen after updating their code, then they're likely to see greater issues than this.
You may want to read the Coding Style guidelines, specifically on variable names (http://docs.moodle.org/dev/Coding_style#Variables). I'd probably name the $old_layout_setting variable as $layout and reuse the variable. I'd also probably rename $course to $courseid to remove any chance of ambiguity:


function put_layout_setting($courseid, $layoutsetting)
  global $DB;
  if ($layout = $DB->get_record('format_topcoll_layout', array('courseid' => $courseid))) {
    $layout->layoutsetting = $layoutsetting;
    $DB->update_record('format_topcoll_layout', $layout);
  } else {
    $layout = new stdClass();
    $layout->courseid = $courseid;
    $layout->layoutsetting = $layoutsetting;
    $DB->insert_record('format_topcoll_layout', $layout);
  }
  return $layout->id;
}

set_layout.php

Remove the trailing php tag
You don't need to put $CFG->wwwroot in the set_url
Rather than wrapping the entire page of code in one if statement for has_capability(), use require_capability() - this should make it easier tor read
You probably don't *need* to test the user_is_editing(), but if you want to, use something like: if (!$PAGE->user_is_editing()) { $courseurl = new moodle_url('/course/view.php', array('id' => $courseid)) redirect($courseurl); }

There may be some others things to consider here - for example you probably want to add require_sesskey() to the page, and include the sesskey in your link to the page

db folder

I assume you created the install.xml using the XML DB editor
Again, I assume that this was mostly generated with the XMLDB editor - it looks fine at a glance

Other things to consider

You may want to include a new capability rather than just using the moodle/course:update capability, but this is really up to you.

All in all, it looks pretty good. You should probably look at moving the images to a pix folder, but I'm not sure of the exact method for doing this for a course format. It should be supported, and then just be a case of using an $OUTPUT->pix_url('t/arrow_down'); or similar.

Good luck

Andrew

Average of ratings: Useful (3)

Re: Please check first time development with DB and Forms

by Gareth J Barnard - Wednesday, 8 February 2012, 10:42 PM

Dear Andrew,

Thank you so much for your time and effort - really appreciated.

I've now created 'https://github.com/gjb2048/moodle-format_topcoll/tree/CONTRIB-3378' for the code and hopefully can fastforward / merge this back into the originating branch. I still do not completly understand rebase but hopefully practice will help.

I've looked at 'pix_url' before and decided against it as I use links in the css and client side Javascript to refer to the images. However with this new graphic I will have a go as the definition of the function in 'outputlib.php' describes the same functionality as 'get_string'.

I'll consider the change to 'require_capability' in 'accesslib.php' as it throws an error. However, I notice that the parameters will be the same and I copied the code from '/course/format/topics/format.php' - so does that need to change? I think I will not create a new capability as the one used for this functionality fits into the umbrella of course changes and I do not want to add more complexity for users where it is not needed - the old climb a mountain when you only needed to climb a hill scenario.

I created the XML by hand in Notepad++ by modifying the one in the Grid format - I did not know about the XML DB editor.

Why do I need 'require_sesskey()' and link at the bottom - how would this help?

Thanks again,

Gareth

Average of ratings: -

Re: Please check first time development with DB and Forms

by Andrew Lyons - Wednesday, 8 February 2012, 11:20 PM

Hi Gareth,

Git is very powerful, but can take a bit of getting your head around. The usage I was suggesting is to perform an interactive rebase to then combine commits together. I wrote a blog post on splitting commits ages ago (http://thamblings.blogspot.com/2010/08/power-of-git-splitting-commit.html). The process of squashing (merging) commits together is very similar. Essentially though:

 

git checkout -b rebase

git rebase -i HEAD~4

(change the 'pick' to a 'squash' for the commits you wish to merge and save + exit your editor)

For the pix_url bits, You should be able to use a CSS syntax like:

 

background: url([[pix:i/arrow_down]])

and in your JavaScript:

 

M.util.image_url('i/arrow_down');

require_capability should work in a very similar way to has_capability as you suggest, but will throw an error instead. The code in the topics/format uses has_capability to only apply logic to certain areas. Thinking about it, you probably don't want to wrap that require_capability in an if statement as the exception will break out anyway:

 

$PAGE->set_context($coursecontext);



require_capability('moodle/course:update', $coursecontext);

if (!$PAGE->user_is_editing()) {

  $courseurl = new moodle_url('/course/view.php', array('id' => $courseid))

  redirect($courseurl);

}

It's generally safest not to write the XML by hand, but to use the XMLDB editor - see the usage documentation on the developer pages at http://docs.moodle.org/dev/XMLDB_editor

It will create the XML for you and ensure that the all of the fields are correct. It's also capable of generating the PHP required in the upgrade.php file.

The sesskey stuff ensures that malicious links don't make changes without your realising (e.g. a bookmark for the show/hide button could be problematic). Thinking about it, in your case it probably isn't required for this.

Andrew

Average of ratings: Useful (2)

Re: Please check first time development with DB and Forms

by Gareth J Barnard - Thursday, 9 February 2012, 3:56 AM

Dear Andrew,

Thanks for the Git information. I will read and digest. I tend to use TortoiseGIT as a GUI to Git to speed things up instead of using the command line (although perfectly happy with command lines).

I looked at http://docs.moodle.org/dev/Themes_2.0_How_to_use_images_within_your_theme and I think it will not work with course formats and in this case as it is not broken I'm not going to fix it. There is no real imperative for making the move as the browser caches the images and css anyway. And indeed I do not even need to refactor the code (in the Martin Fowler sense) before making improvements yet.

I've made the 'require_capability' change as you have suggested - thanks.

If I need to make a DB file in future, I'll use the editor .

And I've added the sesskey as an extra layer to the security onion in a belt and braces fashion.

Thanks,

Gareth

Average of ratings: -

General developer forum

Please check first time development with DB and Forms

Please check first time development with DB and Forms

Re: Please check first time development with DB and Forms

Re: Please check first time development with DB and Forms

Re: Please check first time development with DB and Forms

Re: Please check first time development with DB and Forms

lib.php

set_layout.php

db folder

Other things to consider

Re: Please check first time development with DB and Forms

Re: Please check first time development with DB and Forms

Re: Please check first time development with DB and Forms

Empowering educators to improve our world

Moodle

Support

Get Involved

Contributions

Downloads

Tracker

Development

Empowering educators to improve our world