20 September 2013, 13:33

Hello Tobias,

I'll leave the tech questions for Aaron to answer, but I can help you with your git question. You can find the steps that I take for submitting a first patch and then resubmitting a patch with changes in the Developer Area on the wiki.

Bascially, you can ammend your commit message with git --amend and then just re-submit your patch if you don't want to make any other changes. If you want to make further changes based on the feedback, you can add those with git add and then do git --amend and everything will be pushed to Gerrit again. A new patchset under the same review will be created which keeps the history together nicely.

Cheers

Kristina

23 September 2013, 12:46

Hi Tobias,

Unfortunately, there's not a way to use the install.xml snippet in upgrade.php. What you have to do instead is re-specify the table as an XMLDBTable, and then do a call to create_table() to create it. You should see several examples of this being done further up in upgrade.php. It's kind of awkward that we have to do it this way, but it's what we inherited when we chose to re-use Moodle's DDL library. (Moodle now has an interactive editor for install.xml that generates the PHP to go in upgrade.php, but we haven't ported that over.)

Regarding gerrit, I believe the only editing you can do through gerrit itself is clicking the "Rebase" button, which will attempt to rebase your commit on top of the current master HEAD, which it will refuse to do if there are any rebase conflicts.

So, you will need to use "git commit --amend" and/or "git rebase -i" to edit the commit message (making sure to keep the same Change-Id line!), and then push them back up to gerrit.

It's not actually a "force push", because gerrit uses special refs to do magical things with commits. When you do "make push", using the Mahara makefile, the actual git command it does is "git push gerrit HEAD:refs/publish/master". If you were doing a normal (non-gerrit) push, you'd instead push "HEAD:refs/branches/master". The "refs/publish/..." refspec is a special ref used by gerrit. When you push there, it tells gerrit that you're pushing a patch for review, rather than a commit that goes directly into a branch. Gerrit will then look for a "Change-Id" line in your git commit message, and if it finds one it will use that to identify your commit as a new "patchset" for an existing Gerrit Change. See for instance https://reviews.mahara.org/#/c/2383/ , where we wound up uploading 15 different versions of the commit before merging it in to master.

In your case, because you pushed several commits, it's a little bit trickier when you update a commit that has child commits. But, you'll notice that gerrit has a "Depends on" and "Dependencies" section on each Change page. If you push a new version of a parent commit without pushing new versions of the child commits, it's no big deal. Gerrit will just show the words "OUTDATED" on the "Depends On" section of the child commit's page, and then a reviewer can rebase the child commit onto the latest version of the parent commit.

Actually for our review purposes, it would be best if you could squash all the commits for a single issue down into one commit. (You can squash commits using "git rebase -i"). If you still want to keep them as separate commits in your local workspace so that you can track your development history, you can do that by just doing "git checkout -b" to make a new branch and switch to it, before doing the "git rebase -i" to squash your commits together. Then once you've pushed to gerrit, switch back to your original branch with your unsquashed commits.

Cheers,

Aaron

24 September 2013, 0:27

Hi Kristina and Aaron,

thanks for the help. Seems like I got the two commits merged (so now I can use --amend to change the commit message). I see no need to keep the single commits. 

A note about the create_table: I startet putting the table into lib/db/upgrade.php and adding the cron-jobs in lib/upgrade.php with the result, that on installing Mahara from scratch, lib/db/upgrade.php is not called, so the cronjob got registered and failed adding the entries to the table - which wasn't there. I figure, lib/upgrade.php is used in installation whereas lib/db/upgrade.php is only used if I actually upgrade from an earlier version. 

I added the table-definition into lib/db/install.xml so everything get's created and registered in the installation process. But I also added the create_table-statement and the cron-job/event-listener in lib/db/upgrade.php so they will be added when someone upgrades the system. I tried testing that, checking out  gerrit/1.7_STABLE, installing that version, checking out my branch and running the update. 

I'll do some debugging and then I'll try to push my changes into gerrit and let it do it's magical things 

24 September 2013, 11:42

Hi Tobias,

Yep, that sounds correct so far. You're right, every change you make to the database has to go two places: one for users who are doing a clean install, and one for users who are upgrading from a previous version.

The database upgrades go in the various */db/upgrade.php scripts throughout Mahara. Each plugin gets its own (e.g. artefact/blog/db/upgrade.php), and core stuff that isn't associated with a particular plugin goes in lib/db/upgrade.php. There is also local/db/upgrade.php, which is meant to be for "local" customizations, so that individual sites can run an upgrade.php script without having to hack lib/db/upgrade.php.

The install stuff goes into the plugin, core, or local install.xml for creating new tables on installation, and the various pre-install and post-install functions for everything else. Each plugin can define its own postinst() function that gets called after the plugin's install.xml is processed, e.g. PluginArtefactComment::postinst() in artefact/comment/lib.php. Additionally, there are pre-install and post-install functions in lib/upgrade.php for core things that aren't associated with a particular plugin, and there's one or two pre- and post-install functions for local customizations, in /local/lib.php

Really, lib/upgrade.php should probably be called lib/install.php, because most of the functions in there have to do with installing rather than upgrading.

Cheers,

Aaron

25 September 2013, 22:30

Hi Aaron,

thanks for the clarification.

I just pushed the changes into gerrit. When squashing the commits I had to choose a change-id (in fact I decided to, not sure if I could have commited with both change-ids), so now some of the comments disappeared in gerrit. 

About the reviewer-comments (Kristina): You can now change the notification-delay in the settings under "Site options" - "General settings". I hadn't implemented that in the first place because I was trying to keep the plugin-code as separated from the core-code as possible but as the code is now fully integrated into the system, there is no point to that any more. 

Two things I just stumbled upon: The core-event-handlers only recieve the data-object whereas the plugin-event-handler also are told the name of the event. 

Secondly, when installing the system with the plugin, the eventhandler is called with pages belonging to the root-user (id 0). And then View::get_url dies with a SystemException, so I skipped all notifications that are related to a view belonging to that user. As far as I see, you can't log in with that user anyways, right? 

29 September 2013, 17:02

Hello Tobias,

Great that you updated the code. I put your comments on the review so that anyone reviewing the code / functionality can see them. It's best to keep comments there from now on so we don't cross-post. :-)

Cheers

Kristina

04 September 2013, 22:43

Hello Tobias,

Fantastic that you want to share your code. Having it in core will make your upgrades to later versions easier. :-)

Implementing your feature in Mahara 1.9 will not mean that you can't use it before then already on your instance. You could backport the final implementation to the version of Mahara that you are running to have the best possible compatibility. We do that on some of our instances when clients want to have a feature of an upcoming version. :-)

Cheers

Kristina