Instances #7
Instances #7
Conversation
Conflicts: web/del_entry.php web/edit_entry_handler.php
…into instances Conflicts: db/install.xml db/upgrade.php
|
I added a moodle tracker issue for tracking https://tracker.moodle.org/browse/CONTRIB-7121. |
|
It's going to take me a long time to review those changes, but before I do so a few of points:
|
|
Hallo Davo,
Am Mittwoch, 22. November 2017, 17:26:01 CET schrieb Davo Smith:
It's going to take me a long time to review those changes, but before I do
so a few of points: 1. 17 commits is a lot for me to go through, can you
please squash these into a single commit (or, if that doesn't make sense, a
few commits which each do something self-contained)
I will do a single commit with all commits included.
That will work best for me, because the variable $instance_id is included
at many different positions.
2. Please make sure
that there are no whitespace changes to lines that are otherwise unchanged
(I thought I spotted one, I haven't reviewed much further).
Ok, I will have a look.
3. Please check
that existing functionality is unchanged - e.g. if a site has multiple
block instances and expects them all to point at the same MRBS, then that
will continue to work the same after the changes, if a site has hard-coded
links to a particular week / day / etc. in their booking system, this link
will continue to function without changes.
This is not fully adressed. I will add a global config "default_instance"
which will be used, if no other instance_id is given. This way old links will
continue to work.
4. I know that MRBS does not
have anything in the way of PHPUnit or Behat tests (due to its age), but I
think that when making extensive changes such as this it would be a really
good idea to add some. This may not be easy to do, but I would ask that you
have a look at doing so.
I don't know if I have time to add those.
Thank you for your comments. I will get back to you.
Bye,
Frank
|
… instances Conflicts: web/admin.php
|
Hi Davo, There are no whitespace changes with one exception to my knowledge. In import.php I did need There is a "default_instance", which is the one instance existing at the time of import. Each link without an instance number will point to this instance. I didn't add any test code. I've never done so far and that will need some time for me to learn. |
|
Sorry for the slow reply - I've been pretty busy over the last month and not had any free evenings to sit down and look at Moodle code (after a day of looking at it at work). As you can see - I've made a number of comments inline in the code. My biggest concerns at the moment are: Can I suggest a (hopefully) better alternative solution (which I don't think would be a huge amount of extra work)? Maybe it would be better to add an extra layer of configuration between MRBS and the block instances. So, instead of creating a new MRBS instance for each block instance, have an admin page that allows you to create and configure separate MRBS instances, with the block configuration only being about selecting which MRBS instance that block instance links to. What do you think? Please note that unless solutions are found to a), b) and c) [d) is less important] I'm not going to be able to merge these changes as they will cause unexpected behaviour on existing sites. |
|
Hi Davo,
thank you for your comments. I will need some time to work through and find
solutions. I'll get back to you.
Bye,
Frank
Am Samstag, 23. Dezember 2017, 20:08:36 CET schrieb Davo Smith:
… Sorry for the slow reply - I've been pretty busy over the last month and not
had any free evenings to sit down and look at Moodle code (after a day of
looking at it at work).
As you can see - I've made a number of comments inline in the code.
My biggest concerns at the moment are:
a) you are messing around with existing site configuration during
install/upgrade (changing core roles / adding a block to the front page) b)
existing sites with multiple instances of the block, that currently all
point at the same booking system are going to suddenly find that most of
the blocks don't point at the booking system they were expecting c) What
happens if a block is deleted (by accident or on purpose) - in the current
system you'd just create a new instance of the block and it would be
working again, with your changes, the data would still be sitting in the
database (as far as I can tell), but there would no longer be a way to
access it. This is *very* likely to happen when someone who previously had
the MRBS block on a course page suddenly finds it appearing on the front
page of their site, so immediately removes it again, thereby losing their
only link to their existing room bookings ... d) Backup & restore - as each
MRBS set up is directly linked to a single block, I'd expect that backing
up and restoring a course with that block would copy all the booking data
(or at least have some clear warnings about what is going on).
Can I suggest a (hopefully) better alternative solution (which I don't think
would be a huge amount of extra work)? Maybe it would be better to add an
extra layer of configuration between MRBS and the block instances. So,
instead of creating a new MRBS instance for each block instance, have an
admin page that allows you to create and configure separate MRBS instances,
with the block configuration only being about selecting which MRBS instance
that block instance links to. That way, all existing blocks could continue
to point to the existing MRBS instance, deleting a block would just delete
the link to the MRBS instance - adding a new block and selecting the same
MRBS instance would allow you to continue as you were. You also would have
no need of forcing a block to appear on the front page to act as the
'default' instance. Capability checks would become a bit more complicated -
you'd need to find all the block instances that linked to a given MRBS
instance and check capabilities in each of those (you'd almost certainly
want to store the links in the Moodle cache, so you don't have to re-parse
every block config during every page load). There is also the problem that,
in theory, a teacher could gain editing access to any MRBS instance they
liked, simply by adding the block and pointing it at that MRBS instance.
What do you think?
Please note that unless solutions are found to a), b) and c) [d) is less
important] I'm not going to be able to merge these changes as they will
cause unexpected behaviour on existing sites.
|
|
Not a problem - it looks like a good new feature, it just needs to land without breaking anything on existing sites. |
PHP Deprecated: Methods with the same name as their class will not be constructors in a fuprecated constructor in .../blocks/mrbs/web/mincals.php on line 20
switched from get_all_user_name_fields to \core_user\fields API get_all_user_name_fields deprecated since Moodle 3.11
Update view_entry.php
Hi, I coded multiplce instances version or MRBS.
I do need to do some more testing, especially testing of the import.php functions.
I added code to upgrade.php to create an instance for the existing data in the database, so
that it is not lost.
Each instance is identified by html parameter instance=xxx, which is appended to each link.
I would be happy if you reviewed my changes.