Moodle
  1. Moodle
  2. MDL-44316

Change the tag API to include the contextid and component when tagging an item.

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.6
    • Fix Version/s: 2.7
    • Component/s: Questions, Tags
    • Labels:
    • Testing Instructions:
      Hide
      Test 1
      1. Run "vendor/bin/phpunit tag/tests/taglib_test.php", "vendor/bin/phpunit lib/tests/questionlib_test.php" and "vendor/bin/phpunit course/tests/courselib_test.php" and ensure all passes.
      Test 2
      1. Install a new Moodle site and check that the 'tag_instance' table contains the columns 'component', 'contextid' and 'timecreated' with the following properties.

      'component' - Character field with a length of '100' null allowed.
      'contextid' - Integer field with a length of '10' null allowed.
      'timecreated' - Integer field with a length if '10' null NOT allowed.

      Test 3 (This test must be tested on ALL supported DBs)
      1. Create a Moodle site from the latest stable master branch.
      2. Visit <yoursite>/admin/settings.php?section=optionalsubsystems and ensure 'Enable tags functionality' is checked.
      3. Visit <yoursite>/admin/settings.php?section=blocksettingtags and ensure 'Show course tags' is checked.
      4. Tag a course, wiki page, blog, external blog, user (their interests) and question.
      5. Create a backup of the course you tagged.
      6. Upgrade the Moodle site to the latest integration master and check that the 'tag_instance' table contains the columns 'component', 'contextid' and 'timecreated' with the properties listed above.
      7. Check that the 'component' and 'context' have been populated correctly for the items you tagged.
      8. Restore the course backup and ensure it works and the tags still exist.
      Test 4
      1. Check that you can tag a course, wiki page, blog, external blog, user (their interests) and question on the latest integration master.
      2. Backup and restore a tagged course that contains a tagged wiki and a quiz using a tagged course question and check that it works and the tags remain.
      Test 5
      1. Create a course.
      2. Under 'Course administration' click on 'Question bank' then 'Questions'.
      3. Create a question in the default category for the course and add some tags.
      4. Check the 'tag_instance' table and ensure the correct contextid has been assigned (the contextid will be the context of the question category - you can see this in the question_categories table) for these tags.
      5. Select the question, from the drop down select default for the course but under the category heading, and then click 'Move to >>'.
      6. Check the 'tag_instance' table and ensure the correct contextid has been assigned for these tags.
      7. Click on 'Categories' under the 'Question bank' and move the category containing the question to another section using the arrows.
      8. Check the 'tag_instance' table and ensure the correct contextid has been assigned for these tags.
      9. Edit the question itself and move it to another category.
      10. Delete the question and ensure the 'tag_instance' records have been removed.
      Test 6
      1. Add tags to a wiki post.
      2. Delete the wiki activity.
      3. Check the tag_instances for the activity have been removed.
      Test 7
      1. Add some tags to wiki posts belonging to multiple wiki activities.
      2. Uninstall the wiki module.
      3. Check that all tag_instances for 'mod_wiki' have been removed.
      Show
      Test 1 Run "vendor/bin/phpunit tag/tests/taglib_test.php", "vendor/bin/phpunit lib/tests/questionlib_test.php" and "vendor/bin/phpunit course/tests/courselib_test.php" and ensure all passes. Test 2 Install a new Moodle site and check that the 'tag_instance' table contains the columns 'component', 'contextid' and 'timecreated' with the following properties. 'component' - Character field with a length of '100' null allowed. 'contextid' - Integer field with a length of '10' null allowed. 'timecreated' - Integer field with a length if '10' null NOT allowed. Test 3 (This test must be tested on ALL supported DBs) Create a Moodle site from the latest stable master branch. Visit <yoursite>/admin/settings.php?section=optionalsubsystems and ensure 'Enable tags functionality' is checked. Visit <yoursite>/admin/settings.php?section=blocksettingtags and ensure 'Show course tags' is checked. Tag a course, wiki page, blog, external blog, user (their interests) and question. Create a backup of the course you tagged. Upgrade the Moodle site to the latest integration master and check that the 'tag_instance' table contains the columns 'component', 'contextid' and 'timecreated' with the properties listed above. Check that the 'component' and 'context' have been populated correctly for the items you tagged. Restore the course backup and ensure it works and the tags still exist. Test 4 Check that you can tag a course, wiki page, blog, external blog, user (their interests) and question on the latest integration master. Backup and restore a tagged course that contains a tagged wiki and a quiz using a tagged course question and check that it works and the tags remain. Test 5 Create a course. Under 'Course administration' click on 'Question bank' then 'Questions'. Create a question in the default category for the course and add some tags. Check the 'tag_instance' table and ensure the correct contextid has been assigned (the contextid will be the context of the question category - you can see this in the question_categories table) for these tags. Select the question, from the drop down select default for the course but under the category heading, and then click 'Move to >>'. Check the 'tag_instance' table and ensure the correct contextid has been assigned for these tags. Click on 'Categories' under the 'Question bank' and move the category containing the question to another section using the arrows. Check the 'tag_instance' table and ensure the correct contextid has been assigned for these tags. Edit the question itself and move it to another category. Delete the question and ensure the 'tag_instance' records have been removed. Test 6 Add tags to a wiki post. Delete the wiki activity. Check the tag_instances for the activity have been removed. Test 7 Add some tags to wiki posts belonging to multiple wiki activities. Uninstall the wiki module. Check that all tag_instances for 'mod_wiki' have been removed.
    • Affected Branches:
      MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_27_STABLE
    • Pull Master Branch:
      MDL-44316_master
    • Story Points (Obsolete):
      40
    • Sprint:
      BACKEND Sprint 12

      Description

      We want to update the database table 'tag_instance' so that the contextid and component are stored. This will allow for easy deletion of tags when cleaning up particular areas of Moodle, it will also be consistent with other parts of the Moodle API - eg. the file area.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Tim Hunt added a comment -

            Hi Mark, just saw your Dev chat about DB queries (https://moodle.org/local/chatlogs/index.php?conversationid=14915) and wanted to pass on a tip I learned from Eloy.

            When you add a new style of complex query, it is a good idea to add an abstract version as a test in lib/dml/tests/dml_test.php. That is the easiest way to find out if that style of SQL works in all supported DBs.

            For example, search for "MDL-34657" and "Update records with subquery condition." in that file.

            Of course, that also means that dml_test.php is a good place to look to see what styles of query do work in all DBs.

            Show
            Tim Hunt added a comment - Hi Mark, just saw your Dev chat about DB queries ( https://moodle.org/local/chatlogs/index.php?conversationid=14915 ) and wanted to pass on a tip I learned from Eloy. When you add a new style of complex query, it is a good idea to add an abstract version as a test in lib/dml/tests/dml_test.php. That is the easiest way to find out if that style of SQL works in all supported DBs. For example, search for " MDL-34657 " and "Update records with subquery condition." in that file. Of course, that also means that dml_test.php is a good place to look to see what styles of query do work in all DBs.
            Hide
            Mark Nelson added a comment -

            Thanks Tim for the tips. I have run the queries in PostgreSQL and MySQL successfully and have been told by Petr that it should work in other supported engines as well, so will see if I can find someone to run the upgrade in Oracle to confirm before submitting to integration.

            Show
            Mark Nelson added a comment - Thanks Tim for the tips. I have run the queries in PostgreSQL and MySQL successfully and have been told by Petr that it should work in other supported engines as well, so will see if I can find someone to run the upgrade in Oracle to confirm before submitting to integration.
            Hide
            Petr Skoda added a comment - - edited

            This looks good, the only potential problem might be:

            1/ tag_set('question', $question->id, $fromform->tags, $component, $thiscontext->id); I am not sure why questions would use module component. I suppose this needs to be approved by Tim. If the component is fixed maybe the question/type/questiontypebase.php should use it in where.

            2/ I suppose this change of API should be announced in the general developer forum, I suppose one week should be enough and this can go to integration next week

            thanks!!

            Show
            Petr Skoda added a comment - - edited This looks good, the only potential problem might be: 1/ tag_set('question', $question->id, $fromform->tags, $component, $thiscontext->id); I am not sure why questions would use module component. I suppose this needs to be approved by Tim. If the component is fixed maybe the question/type/questiontypebase.php should use it in where. 2/ I suppose this change of API should be announced in the general developer forum, I suppose one week should be enough and this can go to integration next week thanks!!
            Hide
            Mark Nelson added a comment -

            Hey Petr, thanks for the review.

            Tim Hunt are you able to comment on point 1? Thanks.

            I have created a forum post regarding this issue which you can find by looking at https://moodle.org/mod/forum/discuss.php?d=255636.

            Regards,

            Mark

            Show
            Mark Nelson added a comment - Hey Petr, thanks for the review. Tim Hunt are you able to comment on point 1? Thanks. I have created a forum post regarding this issue which you can find by looking at https://moodle.org/mod/forum/discuss.php?d=255636 . Regards, Mark
            Hide
            Tim Hunt added a comment -

            The question stuff is self-evidently wrong, because the logic is different in different places (question/question.php sets different context/components from all the other places).

            I think it is clear that the context id for question tags must always be the same as the context id for the question category that the question is part of.

            What the right component. I think 'core' is meaningless. There are two sensible choices, 'qtype_xxx', or 'core_question' (which is a recognised component). The tags belong to the core question system, and are the same for all question types, so that suggests to me that 'core_question' is right.

            The code added to public function save_question($question, $form) must be wrong. When saving the question, we save the tags. We should just save them with the right settings in the first place. Saving the wrong data then immediately update it is obviously bad.

            Also, when questions are moved between contexts, you need to update the tags. To work out the place to do that, find the code in question_type_base that moves files to the new context.

            Show
            Tim Hunt added a comment - The question stuff is self-evidently wrong, because the logic is different in different places (question/question.php sets different context/components from all the other places). I think it is clear that the context id for question tags must always be the same as the context id for the question category that the question is part of. What the right component. I think 'core' is meaningless. There are two sensible choices, 'qtype_xxx', or 'core_question' (which is a recognised component). The tags belong to the core question system, and are the same for all question types, so that suggests to me that 'core_question' is right. The code added to public function save_question($question, $form) must be wrong. When saving the question, we save the tags. We should just save them with the right settings in the first place. Saving the wrong data then immediately update it is obviously bad. Also, when questions are moved between contexts, you need to update the tags. To work out the place to do that, find the code in question_type_base that moves files to the new context.
            Hide
            Mark Nelson added a comment -

            Hey Tim, thanks for your feedback.

            The reason the code was placed in that function is because this is called when we update a single question, where we can choose to change the category. I guess we could put this directly into the file question/question.php in the following code and depending on if it is being moved or not use a different context -

            if (!empty($CFG->usetags) && isset($fromform->tags)) {
                // A wizardpage from multipe pages questiontype like calculated may not
                // allow editing the question tags, hence the isset($fromform->tags) test.
                require_once($CFG->dirroot.'/tag/lib.php');
                tag_set('question', $question->id, $fromform->tags, 'core_question', $thiscontext->id);
            }
            

            Then we could also put it in the function question_move_questions_to_category() as well for times we move complete categories.

            Show
            Mark Nelson added a comment - Hey Tim, thanks for your feedback. The reason the code was placed in that function is because this is called when we update a single question, where we can choose to change the category. I guess we could put this directly into the file question/question.php in the following code and depending on if it is being moved or not use a different context - if (!empty($CFG->usetags) && isset($fromform->tags)) { // A wizardpage from multipe pages questiontype like calculated may not // allow editing the question tags, hence the isset($fromform->tags) test. require_once($CFG->dirroot.'/tag/lib.php'); tag_set('question', $question->id, $fromform->tags, 'core_question', $thiscontext->id); } Then we could also put it in the function question_move_questions_to_category() as well for times we move complete categories.
            Hide
            Mark Nelson added a comment -

            I also had to put the code into the function question_move_category_to_context. I am worried I may have missed other places. So far I have found three ways you can update the contextid.

            Show
            Mark Nelson added a comment - I also had to put the code into the function question_move_category_to_context. I am worried I may have missed other places. So far I have found three ways you can update the contextid.
            Hide
            Mark Nelson added a comment -

            Also need to put it in the function question_delete_course_category in case the questions are moved.

            Show
            Mark Nelson added a comment - Also need to put it in the function question_delete_course_category in case the questions are moved.
            Hide
            Mark Nelson added a comment -

            Hey Tim Hunt, I have added some unit tests to ensure that the tag_instance table is updated when the question category context changes. Are you able to check the changes and ensure they are correct? Also, can you think of any other locations I may have missed. Thanks.

            Show
            Mark Nelson added a comment - Hey Tim Hunt , I have added some unit tests to ensure that the tag_instance table is updated when the question category context changes. Are you able to check the changes and ensure they are correct? Also, can you think of any other locations I may have missed. Thanks.
            Hide
            Mark Nelson added a comment -

            Sending this back for peer review.

            Show
            Mark Nelson added a comment - Sending this back for peer review.
            Hide
            Petr Skoda added a comment -

            apart from the misaligned ON and AND keywords in SQL queries it looks to me, but I suppose Tim should review the quiz part because it seems to be the only problematic place. Thanks

            Show
            Petr Skoda added a comment - apart from the misaligned ON and AND keywords in SQL queries it looks to me, but I suppose Tim should review the quiz part because it seems to be the only problematic place. Thanks
            Hide
            CiBoT added a comment -
            Show
            CiBoT added a comment - Results for MDL-44316 Remote repository: https://github.com/markn86/moodle.git Remote branch MDL-44316 _master to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/2138 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/2138/artifact/work/smurf.html
            Hide
            Tim Hunt added a comment -

            Sorry, did not see that you wanted my input here. (I have now added the Questions component, so I should get notified in future.)

            1. DB query $category = $DB->get_record('question_categories', array('id' => $categoryid)); once for each single tag being restored is clearly unacceptable from a performance point of view.

            Store the database record in $this->cachedcateogry, and only query the database if $categoryid != $this->cachedcategory->id. Since restore does one question category at a time, this will actually be optimal.

            2. Upgrade code sets wrong values for the new fields https://github.com/markn86/moodle/compare/master...MDL-44316_master#diff-31dc43dcda8223cb517dfb95643f2ee2R3202

            3. https://github.com/markn86/moodle/compare/master...MDL-44316_master#diff-a22f7fcc9a0c5e983df0ac8d96aaf5e7R485 the comment does not mention the word 'tag' which is confusing. Also, why did you un-wrap the set_field line above?

            4. That whole update should be done in one query. Fetching a long list of ids back to PHP then doing get_in_or_equals is silly.

            5. https://github.com/markn86/moodle/compare/master...MDL-44316_master#diff-a22f7fcc9a0c5e983df0ac8d96aaf5e7R634 can be done with set_field_select, like all the other queries in that bit of code. Please keep in consistent.

            6. https://github.com/markn86/moodle/compare/master...MDL-44316_master#diff-637a32e81c505dc36a25b6eebc52b879R265 $thiscontext->id is wrong. It is not necessarily the new context that the question will be saved in.

            Show
            Tim Hunt added a comment - Sorry, did not see that you wanted my input here. (I have now added the Questions component, so I should get notified in future.) 1. DB query $category = $DB->get_record('question_categories', array('id' => $categoryid)); once for each single tag being restored is clearly unacceptable from a performance point of view. Store the database record in $this->cachedcateogry, and only query the database if $categoryid != $this->cachedcategory->id. Since restore does one question category at a time, this will actually be optimal. 2. Upgrade code sets wrong values for the new fields https://github.com/markn86/moodle/compare/master...MDL-44316_master#diff-31dc43dcda8223cb517dfb95643f2ee2R3202 3. https://github.com/markn86/moodle/compare/master...MDL-44316_master#diff-a22f7fcc9a0c5e983df0ac8d96aaf5e7R485 the comment does not mention the word 'tag' which is confusing. Also, why did you un-wrap the set_field line above? 4. That whole update should be done in one query. Fetching a long list of ids back to PHP then doing get_in_or_equals is silly. 5. https://github.com/markn86/moodle/compare/master...MDL-44316_master#diff-a22f7fcc9a0c5e983df0ac8d96aaf5e7R634 can be done with set_field_select, like all the other queries in that bit of code. Please keep in consistent. 6. https://github.com/markn86/moodle/compare/master...MDL-44316_master#diff-637a32e81c505dc36a25b6eebc52b879R265 $thiscontext->id is wrong. It is not necessarily the new context that the question will be saved in.
            Hide
            Mark Nelson added a comment -

            Thanks Tim!

            1. Done. I also added more to the unit test in question/tests/questionlib_test.php to ensure the backup and restore of tags work correctly.
            2. Fixed.
            3. I have altered the comment. However, I am not too sure what you mean by unwrapping the set_field line. I have not actually moved it's position in the logic.
            4. Done. Not sure how I missed using the contextid field to get the list to update.
            5. Done.
            6. Fixed.

            If you could take another look that would be great. Also, would you be able to confirm that I have not missed any places where it's possible to move a question category?

            Very much appreciated Tim.

            Cheers!

            Show
            Mark Nelson added a comment - Thanks Tim! Done. I also added more to the unit test in question/tests/questionlib_test.php to ensure the backup and restore of tags work correctly. Fixed. I have altered the comment. However, I am not too sure what you mean by unwrapping the set_field line. I have not actually moved it's position in the logic. Done. Not sure how I missed using the contextid field to get the list to update. Done. Fixed. If you could take another look that would be great. Also, would you be able to confirm that I have not missed any places where it's possible to move a question category? Very much appreciated Tim. Cheers!
            Hide
            Tim Hunt added a comment -

            You have rebased the branch already, which means I can't just see what has changed (For future reference, it is better to rebase after the peer reviewer has looked again.)

            Show
            Tim Hunt added a comment - You have rebased the branch already, which means I can't just see what has changed (For future reference, it is better to rebase after the peer reviewer has looked again.)
            Hide
            Tim Hunt added a comment -

            2. I would have expected to find questionlib_test.php in lib/, closer to the code, and where all the other ones like weblib_test.php are. I guess you wanted autoloading to work for it. Hmm. That says something about core APIs that we probably don't want to get into here.

            Acutally, lib/tests/questionlib_test.php already exists. I think you should add to it.

            3. The point about changing the line-wrap is that, you have not actually changed that line of code. But, becuase you changed the whitespace, it shows up in the diff, and it is a complex line of code, to it takes effort to see that you have not changed it. Small point (and you fixed a coding style violation) but it all makes reviewing the code harder).

            4. Your new query is dangerously wrong. You need to limit it to only move core_question tags. https://github.com/markn86/moodle/compare/master...MDL-44316_master#diff-a22f7fcc9a0c5e983df0ac8d96aaf5e7R486

            6. I think this can be improved. The new if statement is ugly. An alternative would be to add

            } else {
               $newcontextid = $category->contextid;
            }
            

            to the first if, instead of setting $movequestion, then later you can just use $newcontextid in the tag set_line. What do you think?

            Show
            Tim Hunt added a comment - 2. I would have expected to find questionlib_test.php in lib/, closer to the code, and where all the other ones like weblib_test.php are. I guess you wanted autoloading to work for it. Hmm. That says something about core APIs that we probably don't want to get into here. Acutally, lib/tests/questionlib_test.php already exists. I think you should add to it. 3. The point about changing the line-wrap is that, you have not actually changed that line of code. But, becuase you changed the whitespace, it shows up in the diff, and it is a complex line of code, to it takes effort to see that you have not changed it. Small point (and you fixed a coding style violation) but it all makes reviewing the code harder). 4. Your new query is dangerously wrong. You need to limit it to only move core_question tags. https://github.com/markn86/moodle/compare/master...MDL-44316_master#diff-a22f7fcc9a0c5e983df0ac8d96aaf5e7R486 6. I think this can be improved. The new if statement is ugly. An alternative would be to add } else { $newcontextid = $category->contextid; } to the first if, instead of setting $movequestion, then later you can just use $newcontextid in the tag set_line. What do you think?
            Hide
            Tim Hunt added a comment -

            Sorry, had to finish typing that previous comment in a hurry. The summary is I think this is really close to integration. Feel free to catch me on Jabber to turn around any last checks quickly.

            Show
            Tim Hunt added a comment - Sorry, had to finish typing that previous comment in a hurry. The summary is I think this is really close to integration. Feel free to catch me on Jabber to turn around any last checks quickly.
            Hide
            Mark Nelson added a comment -

            Thanks Tim,

            All points addressed except for number 6.

            I did remove the if statement but did not like the variable name $newcontextid as it implies we will be adding the tags to a new context, when that won't always be the case. I have gone with $contextid instead.

            If you could have one re-look over that would be awesome. Sorry, my habit of rebasing the changes before you had a look occurred again.

            Thanks Tim.

            Show
            Mark Nelson added a comment - Thanks Tim, All points addressed except for number 6. I did remove the if statement but did not like the variable name $newcontextid as it implies we will be adding the tags to a new context, when that won't always be the case. I have gone with $contextid instead. If you could have one re-look over that would be awesome. Sorry, my habit of rebasing the changes before you had a look occurred again. Thanks Tim.
            Hide
            Tim Hunt added a comment -

            Mark, if you want me to look at this again, please give me a patch that shows the difference between the code now, and the code as it was yesterday.

            I don't have time to work through 600 lines of changes again.

            Show
            Tim Hunt added a comment - Mark, if you want me to look at this again, please give me a patch that shows the difference between the code now, and the code as it was yesterday. I don't have time to work through 600 lines of changes again.
            Hide
            Tim Hunt added a comment -

            OK, I found a very old branch of Mark's on his github, and rebase. Then I rebased the branch given above. Compare https://github.com/timhunt/moodle/commits/tagold and https://github.com/timhunt/moodle/commits/tagnew - I don't think there is a github URL for that.

            Show
            Tim Hunt added a comment - OK, I found a very old branch of Mark's on his github, and rebase. Then I rebased the branch given above. Compare https://github.com/timhunt/moodle/commits/tagold and https://github.com/timhunt/moodle/commits/tagnew - I don't think there is a github URL for that.
            Hide
            Tim Hunt added a comment -

            OK, I have just wasted more time on this than I can really afford, but I think we are good to go.

            Except that this it not rebase on to the latest weekly master. Once you have done that, you can submit for integration.

            Show
            Tim Hunt added a comment - OK, I have just wasted more time on this than I can really afford, but I think we are good to go. Except that this it not rebase on to the latest weekly master. Once you have done that, you can submit for integration.
            Hide
            Mark Nelson added a comment -

            Thanks for your time Tim. In the future I will not rebase my changes so you can easily review. Submitting to integration.

            Show
            Mark Nelson added a comment - Thanks for your time Tim. In the future I will not rebase my changes so you can easily review. Submitting to integration.
            Hide
            CiBoT added a comment -

            Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

            Show
            CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
            Hide
            Marina Glancy added a comment -

            thanks Mark, integrated in master

            one little thing that I've noticed:
            https://github.com/markn86/moodle/compare/master...MDL-44316_master#diff-9f8004472b4dd966811fc19aa7d749a9R3284
            default value for cachedcategory is array() but later you expect it to be an object

            Considering that you are in another timezone and tomorrow is the testing day, I added a correcting commit myself

            Show
            Marina Glancy added a comment - thanks Mark, integrated in master one little thing that I've noticed: https://github.com/markn86/moodle/compare/master...MDL-44316_master#diff-9f8004472b4dd966811fc19aa7d749a9R3284 default value for cachedcategory is array() but later you expect it to be an object Considering that you are in another timezone and tomorrow is the testing day, I added a correcting commit myself
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Ho,

            I'm sorry this is failing badly some CI tests (comparing installed/upgraded databases). Please fix it ASAP!

            I'd say there is an inconsistent default in tag_instance->contextid and also the contextid index is missing on upgrade.

            Report: http://social.srv.in.moodle.com/view/master/job/25.%20Compare%20installed-upgraded%20DB%20from%2022_STABLE%20(master)/2329/artifact/compare_databases_master_MOODLE_22_STABLE.txt
            Ciao

            Show
            Eloy Lafuente (stronk7) added a comment - Ho, I'm sorry this is failing badly some CI tests (comparing installed/upgraded databases). Please fix it ASAP! I'd say there is an inconsistent default in tag_instance->contextid and also the contextid index is missing on upgrade. Report: http://social.srv.in.moodle.com/view/master/job/25.%20Compare%20installed-upgraded%20DB%20from%2022_STABLE%20(master)/2329/artifact/compare_databases_master_MOODLE_22_STABLE.txt Ciao
            Hide
            Marina Glancy added a comment -

            failing test

            Show
            Marina Glancy added a comment - failing test
            Hide
            Mark Nelson added a comment -

            I have created https://github.com/markn86/moodle/commit/b846694b048fcb550c04fc4ae4866a6d095c4f60 to resolve this issue. Would be great if you could have a look Marina before it gets integrated. Thanks.

            Show
            Mark Nelson added a comment - I have created https://github.com/markn86/moodle/commit/b846694b048fcb550c04fc4ae4866a6d095c4f60 to resolve this issue. Would be great if you could have a look Marina before it gets integrated. Thanks.
            Hide
            Mark Nelson added a comment -

            After discussing with Marina in chat it was decided a new upgrade process was required, which you can find at - https://github.com/markn86/moodle/commit/53a1ace449321cb6c6310e9b1fd0d8eff9804587.

            Show
            Mark Nelson added a comment - After discussing with Marina in chat it was decided a new upgrade process was required, which you can find at - https://github.com/markn86/moodle/commit/53a1ace449321cb6c6310e9b1fd0d8eff9804587 .
            Hide
            Mark Nelson added a comment - - edited
            Show
            Mark Nelson added a comment - - edited Rebased on integration - https://github.com/markn86/moodle/commit/0a341617e4d15f92800a6cc15a8bbe23602c1f74 .
            Hide
            Marina Glancy added a comment -

            Thanks Mark, integrated, ready for testing

            Show
            Marina Glancy added a comment - Thanks Mark, integrated, ready for testing
            Hide
            Marina Glancy added a comment -

            failing

            Default exception handler: column "tag_instance-&gt;contextid" cannot be modified. Dependency found with index "mdl_taginst_con_ix (contextid)" Debug: 
            Error code: ddldependencyerror
            * line 720 of /lib/ddl/database_manager.php: ddl_dependency_exception thrown
            * line 640 of /lib/ddl/database_manager.php: call to database_manager->check_field_dependencies()
            * line 3333 of /lib/db/upgrade.php: call to database_manager->change_field_default()
            * line 1560 of /lib/upgradelib.php: call to xmldb_main_upgrade()
            * line 165 of /admin/cli/upgrade.php: call to upgrade_core()
            

            Show
            Marina Glancy added a comment - failing Default exception handler: column "tag_instance-&gt;contextid" cannot be modified. Dependency found with index "mdl_taginst_con_ix (contextid)" Debug: Error code: ddldependencyerror * line 720 of /lib/ddl/database_manager.php: ddl_dependency_exception thrown * line 640 of /lib/ddl/database_manager.php: call to database_manager->check_field_dependencies() * line 3333 of /lib/db/upgrade.php: call to database_manager->change_field_default() * line 1560 of /lib/upgradelib.php: call to xmldb_main_upgrade() * line 165 of /admin/cli/upgrade.php: call to upgrade_core()
            Hide
            Marina Glancy added a comment -

            pulled fix from Petr https://github.com/skodak/moodle/commits/tagcontext
            back to testing

            Show
            Marina Glancy added a comment - pulled fix from Petr https://github.com/skodak/moodle/commits/tagcontext back to testing
            Hide
            Damyon Wiese added a comment -

            Finished this long long test. No errors found - all looks fine.

            Before Petrs last patch I got warnings from XMLDB about the defaults, but updating to latest integration solved them.

            Passing.

            Show
            Damyon Wiese added a comment - Finished this long long test. No errors found - all looks fine. Before Petrs last patch I got warnings from XMLDB about the defaults, but updating to latest integration solved them. Passing.
            Hide
            Mark Nelson added a comment -

            Thanks guys, sorry for the hassle.

            Show
            Mark Nelson added a comment - Thanks guys, sorry for the hassle.
            Hide
            Petr Skoda added a comment -

            No worries Mark, it is great to have the API finally fixed, thanks everybody!

            Show
            Petr Skoda added a comment - No worries Mark, it is great to have the API finally fixed, thanks everybody!
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Clothes and manners do
            not make the man; but,
            when he is made, they
            greatly improve his appearance.

            ---- Henry Ward Beecher

            What a week, your changes are now part of Moodle, well done!

            Closing, thanks!

            Show
            Eloy Lafuente (stronk7) added a comment - Clothes and manners do not make the man; but, when he is made, they greatly improve his appearance. ---- Henry Ward Beecher What a week, your changes are now part of Moodle, well done! Closing, thanks!
            Hide
            Ankit Agarwal added a comment -

            This caused a regression MDL-45258

            Show
            Ankit Agarwal added a comment - This caused a regression MDL-45258

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Agile