Moodle Community Sites
  1. Moodle Community Sites
  2. MDLSITE-1152

Show old version of a module description (other fields) if any while new version awaits moderator's approval

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Component/s: moodle.org
    • Labels:
      None
    • Rank:
      25901

      Description

      Right now when you edit you module description (or some field), you could make it inaccessible for other people (and not even know about it, since it will be accessible for you) for an undefined period of time. That's very bad: you add something by people's request ... and they see you module disappeared at all (no even warning that it's under moderation).

      We all know that Moodle Team is a busy people and moderator couldn't approve every little change fast. So the system should step in their place. If there is an old, moderated version, it should be showed instead of new one. Or, at least, module shouldn't disappear from other people and unlogged in search, but show message that it's under moderation instead.

      1. approval_fix_with_nulls.diff
        17 kB
        Davo Smith
      2. finished_approval_fix.diff
        14 kB
        Davo Smith
      3. partial_approval_fix.diff
        14 kB
        Davo Smith

        Issue Links

          Activity

          Oleg Sychev created issue -
          Helen Foster made changes -
          Field Original Value New Value
          Link This issue will be resolved by MDLSITE-571 [ MDLSITE-571 ]
          Hide
          Davo Smith added a comment -

          Here is a partially complete patch that goes some way to fixing this problem.

          What works:

          • editing records leaves the old version visible to everyone, except you and teachers
          • you and teachers see the new version in the 'view list' tab
          • approving records deletes the old version and makes the new version visible to everyone

          What does NOT work:

          • the 'view single' tab displays the old version to teachers and to the person who edited it (when paging through all the entries)
          • clicking on the record to jump straight into the 'view single' tab, displays the new record, but with the page selection broken

          What this needs:
          some clever SQL work, or better post-processing of the results after they are returned
          possibly also needs an index adding to the 'replaces' field speed up searches

          I'll look at it again later today, if I get a chance, but I thought posting this early version might help push the conversation on a little.

          Show
          Davo Smith added a comment - Here is a partially complete patch that goes some way to fixing this problem. What works: editing records leaves the old version visible to everyone, except you and teachers you and teachers see the new version in the 'view list' tab approving records deletes the old version and makes the new version visible to everyone What does NOT work: the 'view single' tab displays the old version to teachers and to the person who edited it (when paging through all the entries) clicking on the record to jump straight into the 'view single' tab, displays the new record, but with the page selection broken What this needs: some clever SQL work, or better post-processing of the results after they are returned possibly also needs an index adding to the 'replaces' field speed up searches I'll look at it again later today, if I get a chance, but I thought posting this early version might help push the conversation on a little.
          Davo Smith made changes -
          Attachment partial_approval_fix.diff [ 22444 ]
          Hide
          Oleg Sychev added a comment -

          Davo - I'll see that patch as fast as I can, but unfortunately it's probably 3-5 days away.

          However. I could try to write clever SQL faster if you tell me what you need, i.e. "I have tables X, Y and Z and I want to get ......" carefully describing what data and under which circumstances you want. That's faster than reviewing 14kb patch.

          Show
          Oleg Sychev added a comment - Davo - I'll see that patch as fast as I can, but unfortunately it's probably 3-5 days away. However. I could try to write clever SQL faster if you tell me what you need, i.e. "I have tables X, Y and Z and I want to get ......" carefully describing what data and under which circumstances you want. That's faster than reviewing 14kb patch.
          Hide
          Davo Smith added a comment -

          The clever SQL is the complex query already built by the standard view entries code, but removing any fields whose IDs are stored in the 'replaces' field of any of the other records that are returned by the initial query.

          I'm not sure if it is possible to create a single SQL query that then removes results from itself based on other results that were returned by the original query (especially when limiting the number of returned records).

          Show
          Davo Smith added a comment - The clever SQL is the complex query already built by the standard view entries code, but removing any fields whose IDs are stored in the 'replaces' field of any of the other records that are returned by the initial query. I'm not sure if it is possible to create a single SQL query that then removes results from itself based on other results that were returned by the original query (especially when limiting the number of returned records).
          Hide
          Davo Smith added a comment -

          This is a fixed version of the initial patch I uploaded. It now seems to be working properly, but could do with a thorough code review and testing.

          How it works:

          • When you edit an approved record (as a student, with approval required), it creates a new record and sets the 'replaces' field to point at the original record (if such a record already exists, it just updates it)
          • When viewing records, the SQL query has been changed to remove any records that are 'replaced' by other visible records (i.e. teachers / original authors have the original record hidden and the unapproved replacement shown instead; everyone else just sees the original record)
          • When a replacement record is approved, the original content is deleted, the new content is updated to point at the old record, then the new record is deleted (this means that the URL pointing at the entry will stay the same, but that any user editing one of their entries at the same time as it is approved may get an error message).

          Hope that the fix is sensible.

          Show
          Davo Smith added a comment - This is a fixed version of the initial patch I uploaded. It now seems to be working properly, but could do with a thorough code review and testing. How it works: When you edit an approved record (as a student, with approval required), it creates a new record and sets the 'replaces' field to point at the original record (if such a record already exists, it just updates it) When viewing records, the SQL query has been changed to remove any records that are 'replaced' by other visible records (i.e. teachers / original authors have the original record hidden and the unapproved replacement shown instead; everyone else just sees the original record) When a replacement record is approved, the original content is deleted, the new content is updated to point at the old record, then the new record is deleted (this means that the URL pointing at the entry will stay the same, but that any user editing one of their entries at the same time as it is approved may get an error message). Hope that the fix is sensible.
          Davo Smith made changes -
          Attachment finished_approval_fix.diff [ 22446 ]
          Hide
          Oleg Sychev added a comment -

          Clever is SQL is possible but would not be efficient on large databases. The problem is in data structure.

          The best way to handle this would be actually to add new table "data_record_versions" instead of new field "replaces" which is hard to handle when there are a chain of replaces. userid, groupid and dataid should remain in data_record, time_created may too - time_modified and approved should be moved to the new table, and fields should use it's id, not from data_record.

          Than you could easily build query to get latest approved version of record, or latest at all - just use GROUP BY with MIN (MAX) aggregation function.

          Show
          Oleg Sychev added a comment - Clever is SQL is possible but would not be efficient on large databases. The problem is in data structure. The best way to handle this would be actually to add new table "data_record_versions" instead of new field "replaces" which is hard to handle when there are a chain of replaces. userid, groupid and dataid should remain in data_record, time_created may too - time_modified and approved should be moved to the new table, and fields should use it's id, not from data_record. Than you could easily build query to get latest approved version of record, or latest at all - just use GROUP BY with MIN (MAX) aggregation function.
          Hide
          Davo Smith added a comment -

          OK, that sounds like a more sensible approach - I'll look into creating an amended patch that works a bit more like that.

          One thing that isn't really needed is a chain of 'replaces' - there should only ever be a maximum of 2 versions: the 'approved' version that everyone can see, the 'pending' version that only teachers/originator can see.

          Show
          Davo Smith added a comment - OK, that sounds like a more sensible approach - I'll look into creating an amended patch that works a bit more like that. One thing that isn't really needed is a chain of 'replaces' - there should only ever be a maximum of 2 versions: the 'approved' version that everyone can see, the 'pending' version that only teachers/originator can see.
          Hide
          Helen Foster added a comment -

          Added our contributions coordinator Anthony as a watcher in case he'd like to comment.

          Show
          Helen Foster added a comment - Added our contributions coordinator Anthony as a watcher in case he'd like to comment.
          Hide
          Oleg Sychev added a comment -

          Davo - Yes, if you don't have time to implement "versions" solution (which would have many other potential good uses), you could limit chain for 2: last approved and last at all (editing unapproved versions just erases previous unapproved one). Then you can easily filter them in both ways, since you could set "replaces" to NULL for one of them.

          Show
          Oleg Sychev added a comment - Davo - Yes, if you don't have time to implement "versions" solution (which would have many other potential good uses), you could limit chain for 2: last approved and last at all (editing unapproved versions just erases previous unapproved one). Then you can easily filter them in both ways, since you could set "replaces" to NULL for one of them.
          Hide
          Davo Smith added a comment -

          Oleg - that's more or less what the patch I've supplied does, except the filtering isn't quite that simple, as you need to find all the records that match your criteria (i.e. active search, current group, approval status - approved, not approved & owned by you, not approved) then find any which match your criteria AND have the 'replaces' variable set, then remove the items in the first list that are overwritten by the items in the second list.

          For a student, just getting the records with 'replaces=0' will miss out any records they have edited themselves (and should be able to see, before approval).
          For a teacher, just getting the records with 'replaces!=0' will miss out any records that are unchanged since they were last approved.

          When I have time (not tonight) I'll try creating a patch which uses a 'data_version' table, but I'm a bit worried that it will require a big change during the upgrade process and will break the restoring of any old backups. The current patch should (subject to any bugs I've missed), cleanly work with any old records and old backup files (by exhibiting the old behaviour, until the entries are next approved).

          Show
          Davo Smith added a comment - Oleg - that's more or less what the patch I've supplied does, except the filtering isn't quite that simple, as you need to find all the records that match your criteria (i.e. active search, current group, approval status - approved, not approved & owned by you, not approved) then find any which match your criteria AND have the 'replaces' variable set, then remove the items in the first list that are overwritten by the items in the second list. For a student, just getting the records with 'replaces=0' will miss out any records they have edited themselves (and should be able to see, before approval). For a teacher, just getting the records with 'replaces!=0' will miss out any records that are unchanged since they were last approved. When I have time (not tonight) I'll try creating a patch which uses a 'data_version' table, but I'm a bit worried that it will require a big change during the upgrade process and will break the restoring of any old backups. The current patch should (subject to any bugs I've missed), cleanly work with any old records and old backup files (by exhibiting the old behaviour, until the entries are next approved).
          Hide
          Oleg Sychev added a comment -

          Davo - right now Moodle 2.0 doesn't support 1.9 backups, so it's one of the best moments to break compatibility with them Thought 2.1 with it's module rework will break it eventually anyway. Versions could have many applications: for example it's easy to store multiple attempts with them, where teacher grades and gives comments what wrong, student change database record, gets graded once more etc - preserving full history. But it is major rework, so it's up to you whether you have time for it. Right now I am overburdened at work, but this could change later.

          I'd prefer using NULL instead if 0 for replaces, than NOTNULL should be set to true for this column and there should be no default.

          I could write SQL for teacher and student's cases, but it is already complex generation there, I don't want to make it more complicated. One idea how to simplify it is to have 3 possibilities for replaces
          1. positive id - this item replaces another one with this id
          2. zero or NULL - this item doesn't replaces another one and don't have replacements
          3. negative id - this item has replacement (with this id)

          Than it would be easy for you to identify both item that are replacement and those that have replacement, while now it is complex.

          Show
          Oleg Sychev added a comment - Davo - right now Moodle 2.0 doesn't support 1.9 backups, so it's one of the best moments to break compatibility with them Thought 2.1 with it's module rework will break it eventually anyway. Versions could have many applications: for example it's easy to store multiple attempts with them, where teacher grades and gives comments what wrong, student change database record, gets graded once more etc - preserving full history. But it is major rework, so it's up to you whether you have time for it. Right now I am overburdened at work, but this could change later. I'd prefer using NULL instead if 0 for replaces, than NOTNULL should be set to true for this column and there should be no default. I could write SQL for teacher and student's cases, but it is already complex generation there, I don't want to make it more complicated. One idea how to simplify it is to have 3 possibilities for replaces 1. positive id - this item replaces another one with this id 2. zero or NULL - this item doesn't replaces another one and don't have replacements 3. negative id - this item has replacement (with this id) Than it would be easy for you to identify both item that are replacement and those that have replacement, while now it is complex.
          Hide
          Davo Smith added a comment -

          Oleg - here is a new version of the patch.

          I've not implemented the 'versioning' system suggested, but I have taken on board your suggestion about using
          replaces=NULL for unchanged records,
          replaces=-id for records that are awaiting approval to be replaced by the record 'id' and
          replaces=id for records that are awaiting approval to replace the record 'id'.

          This has greatly simplified the SQL statement (I no longer need a r.id NOT IN [subquery] statement to get it to work).

          I think this is all working, but here are the areas I've changed, in case I've missed something:

          • viewing (to show original / new version as appropriate)
          • editing (to create a provisional copy, if not edited by a teacher)
          • deleting (to remove the original as well as the new version)
          • backup/restore
          • approval (to copy the data from the new version and then remove the 'new' record)
          • RSS feed generation (to show replacement records if 'requires approval' is switched off)

          One thought - on the back of this, would it be worth giving the teacher a 'revert' button, if they do not want to approve a particular change?
          We could also do with a message 'this record is awaiting moderation - other users [cannot see it / will see the old version] until it is approved' (obviously only the appropriate version of that message would be displayed).

          I won't attempt either of these extra changes until / unless the first set are agreed to.

          Show
          Davo Smith added a comment - Oleg - here is a new version of the patch. I've not implemented the 'versioning' system suggested, but I have taken on board your suggestion about using replaces=NULL for unchanged records, replaces=-id for records that are awaiting approval to be replaced by the record 'id' and replaces=id for records that are awaiting approval to replace the record 'id'. This has greatly simplified the SQL statement (I no longer need a r.id NOT IN [subquery] statement to get it to work). I think this is all working, but here are the areas I've changed, in case I've missed something: viewing (to show original / new version as appropriate) editing (to create a provisional copy, if not edited by a teacher) deleting (to remove the original as well as the new version) backup/restore approval (to copy the data from the new version and then remove the 'new' record) RSS feed generation (to show replacement records if 'requires approval' is switched off) One thought - on the back of this, would it be worth giving the teacher a 'revert' button, if they do not want to approve a particular change? We could also do with a message 'this record is awaiting moderation - other users [cannot see it / will see the old version] until it is approved' (obviously only the appropriate version of that message would be displayed). I won't attempt either of these extra changes until / unless the first set are agreed to.
          Davo Smith made changes -
          Attachment approval_fix_with_nulls.diff [ 22469 ]
          Hide
          Anthony Borrow added a comment -

          I've not looked at the patches but it does look like some of the database fields like content2 content3 or content4 could be used for this purpose. It appears that lat/long field type is using content1. Peace - Anthony

          Show
          Anthony Borrow added a comment - I've not looked at the patches but it does look like some of the database fields like content2 content3 or content4 could be used for this purpose. It appears that lat/long field type is using content1. Peace - Anthony
          Hide
          Oleg Sychev added a comment -

          Anthony, that affects all record, not just one field. And we can't be sure all these contentX fields won't be used by some field type now or later. The thing you recommended is a last resort when you have absolutely no time, but need to do work quickly.

          The solution Davo implemented is better and more suited for not having problems with further development of database module.

          Show
          Oleg Sychev added a comment - Anthony, that affects all record, not just one field. And we can't be sure all these contentX fields won't be used by some field type now or later. The thing you recommended is a last resort when you have absolutely no time, but need to do work quickly. The solution Davo implemented is better and more suited for not having problems with further development of database module.
          Hide
          Anthony Borrow added a comment -

          Oleg - Absolutely, I was thinking of quick and easy patch without modification to the database which is certainly a short sighted fix. Long term solution is to add a field to the database. I had not looked at the patch but I do see that there is a new field created which would be helpful. With this functionality, a teacher could either approve the new data or rollback to the previous data. In any case, this would be an improvement to the database activity module and open up some interesting possibilities. Peace - Anthony

          Show
          Anthony Borrow added a comment - Oleg - Absolutely, I was thinking of quick and easy patch without modification to the database which is certainly a short sighted fix. Long term solution is to add a field to the database. I had not looked at the patch but I do see that there is a new field created which would be helpful. With this functionality, a teacher could either approve the new data or rollback to the previous data. In any case, this would be an improvement to the database activity module and open up some interesting possibilities. Peace - Anthony
          Hide
          Davo Smith added a comment -

          The patch I proposed works by adding a field to the 'data_records' table (to indicate that this record replaces/is replaced by another record) and then duplicating the 'data_content' entries for that record (filling them with the new data). On approval, the contents of the new 'data_content' entries are copied back over the originals and the new 'data_records' entry is discarded.

          Based on your suggestion an alternative implementation would be to add 5 extra fields to the 'data_content' - 'content_new', 'content1_new', 'content2_new', 'content3_new', 'content4_new'. The 'new' entry could be stored in these, with the choice of showing the new/old version being based on the ownership / capabilities of the viewer.

          This would get around the untidiness of creating a temporary record for unapproved entries, but does bloat the 'data_content' table a bit.

          Obviously, at the moment only 'content' and 'content1' appear to be used (content1 appears to be used as a FORMAT_* field for 'textarea' entries), but this proposal will retain the flexibility for future use of the other fields, by other 'data_fields' types.

          I hope that make sense (it gets very confusing talking about records and 'data_records' entries, about fields and 'data_fields' entries).

          Show
          Davo Smith added a comment - The patch I proposed works by adding a field to the 'data_records' table (to indicate that this record replaces/is replaced by another record) and then duplicating the 'data_content' entries for that record (filling them with the new data). On approval, the contents of the new 'data_content' entries are copied back over the originals and the new 'data_records' entry is discarded. Based on your suggestion an alternative implementation would be to add 5 extra fields to the 'data_content' - 'content_new', 'content1_new', 'content2_new', 'content3_new', 'content4_new'. The 'new' entry could be stored in these, with the choice of showing the new/old version being based on the ownership / capabilities of the viewer. This would get around the untidiness of creating a temporary record for unapproved entries, but does bloat the 'data_content' table a bit. Obviously, at the moment only 'content' and 'content1' appear to be used (content1 appears to be used as a FORMAT_* field for 'textarea' entries), but this proposal will retain the flexibility for future use of the other fields, by other 'data_fields' types. I hope that make sense (it gets very confusing talking about records and 'data_records' entries, about fields and 'data_fields' entries).
          Hide
          Anthony Borrow added a comment -

          I've added Sam as a watcher so that he can take this into consideration in the specs for the improved Modules and Plugins system. The basic idea is that we hold on to and display any old data while an entry is awaiting re-approval by a moderator. Peace - Anthony

          Show
          Anthony Borrow added a comment - I've added Sam as a watcher so that he can take this into consideration in the specs for the improved Modules and Plugins system. The basic idea is that we hold on to and display any old data while an entry is awaiting re-approval by a moderator. Peace - Anthony
          Hide
          Helen Foster added a comment -

          Resolving as fixed since http://moodle.org/plugins enables people to update their entries without them needing re-approving.

          Show
          Helen Foster added a comment - Resolving as fixed since http://moodle.org/plugins enables people to update their entries without them needing re-approving.
          Helen Foster made changes -
          Status Open [ 1 ] Resolved [ 5 ]
          Assignee Helen Foster [ tsala ] moodle.com [ moodle.com ]
          Resolution Fixed [ 1 ]
          Hide
          Joseph Rézeau added a comment -

          GOOD !

          Show
          Joseph Rézeau added a comment - GOOD !
          Martin Dougiamas made changes -
          Workflow jira [ 40579 ] SITES Full Workflow [ 126355 ]
          Martin Dougiamas made changes -
          Status Resolved [ 5 ] Closed [ 6 ]

            People

            • Votes:
              8 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development