Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-372

Forum: Sticky/Pinned discussions

    Details

    • Database:
      Any
    • Testing Instructions:
      Hide
      1. Create a forum.
      2. Add a discussion topic.
      3. Add a second discussion topic.
      4. add a third discussion topic
      5. add a fourth discussion topic
      6. Pin the first topic. Verify that it sorts to the top and is labeled "pinned."
      7. Pin the fourth discussion topic. Verify that it sorts to the top above the first pinned topic.

      Repeat above using "timed" discussions

      Test the above on different forum types: blog
      Test on multiple databases: oracle, postgres, mssql, mysql
      Backup and restore an activity and verify that it retains the pinned status for discussion

      Show
      Create a forum. Add a discussion topic. Add a second discussion topic. add a third discussion topic add a fourth discussion topic Pin the first topic. Verify that it sorts to the top and is labeled "pinned." Pin the fourth discussion topic. Verify that it sorts to the top above the first pinned topic. Repeat above using "timed" discussions Test the above on different forum types: blog Test on multiple databases: oracle, postgres, mssql, mysql Backup and restore an activity and verify that it retains the pinned status for discussion
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_23_STABLE, MOODLE_26_STABLE, MOODLE_28_STABLE, MOODLE_30_STABLE
    • Fixed Branches:
      MOODLE_31_STABLE
    • Pull Master Branch:
      MDL-372-int-master

      Description

      I would like to be able to pin a topic in a discussion. This feature should only be available to admin and the teachers.

        Gliffy Diagrams

        1. MDL-372.jpg
          18 kB
        2. MDL-372-pin.jpg
          12 kB
        3. pin.png
          0.8 kB
        4. unpin.png
          0.9 kB

          Issue Links

            Activity

            Hide
            dougiamas Martin Dougiamas added a comment -

            From Martin Dougiamas (martin at moodle.com) Friday, 25 April 2003, 09:55 AM:

            Which would pin it at the top of a forum?

            From Gunther Dippe (dippe at ncm.gu.se) Friday, 25 April 2003, 02:51 PM:

            You got it!

            From Gustav Delius (gwd2 at york.ac.uk) Wednesday, 15 March 2006, 03:39 AM:

            Re-classified as forum feature request

            Show
            dougiamas Martin Dougiamas added a comment - From Martin Dougiamas (martin at moodle.com) Friday, 25 April 2003, 09:55 AM: Which would pin it at the top of a forum? From Gunther Dippe (dippe at ncm.gu.se) Friday, 25 April 2003, 02:51 PM: You got it! From Gustav Delius (gwd2 at york.ac.uk) Wednesday, 15 March 2006, 03:39 AM: Re-classified as forum feature request
            Hide
            dougiamas Martin Dougiamas added a comment -

            Assigning to me temporarily because Vy-Shane no longer works for Moodle HQ.

            Show
            dougiamas Martin Dougiamas added a comment - Assigning to me temporarily because Vy-Shane no longer works for Moodle HQ.
            Hide
            niallj Niall Julian added a comment -

            I'm surprised we don't have a sticky option already. It would be a really useful addition to the forums. I have quite a few important posts that need to be stickies and the only way to do this right now is go in and make an edit to the topic. This can get very time consuming.

            Show
            niallj Niall Julian added a comment - I'm surprised we don't have a sticky option already. It would be a really useful addition to the forums. I have quite a few important posts that need to be stickies and the only way to do this right now is go in and make an edit to the topic. This can get very time consuming.
            Hide
            stuartrmealor Stuart R Mealor added a comment -

            Yeah, considering how vital Forums are to the 'best practice' use of Moodle, it seems strange this facility (so comom on bulleting boards for many years) is missing. Would be a good addition

            Show
            stuartrmealor Stuart R Mealor added a comment - Yeah, considering how vital Forums are to the 'best practice' use of Moodle, it seems strange this facility (so comom on bulleting boards for many years) is missing. Would be a good addition
            Hide
            dougiamas Martin Dougiamas added a comment -

            I guess it's more important in bulletin boards because they have just forums only.

            Moodle has other ways to capture information for later use:

            • you could move the discussion to a special "sticky" forum
            • you could capture the data in a wiki, glossary, or database

            I'm not saying we shouldn't do it, just explaining that alternatives existed.

            Show
            dougiamas Martin Dougiamas added a comment - I guess it's more important in bulletin boards because they have just forums only. Moodle has other ways to capture information for later use: you could move the discussion to a special "sticky" forum you could capture the data in a wiki, glossary, or database I'm not saying we shouldn't do it, just explaining that alternatives existed.
            Hide
            hinkelman Don Hinkelman added a comment -

            >> Moodle has other ways to capture information for later use:
            >> - you could move the discussion to a special "sticky" forum
            >> - you could capture the data in a wiki, glossary, or database

            Are these two new features planned for Moodle 2.0? If so, I believe the "pinned topics" request would be handled, if an admin or teacher could redesignate an ordinary forum into a "Sticky" forum. Then be able select the topic(s) which would become "sticky" (fixed at the top of the list of topics). Presumably, if more than one forum topic is selected as sticky, the teacher could fix the order of the sticky topics (first, second, third...).

            On the data capture feature, that is another great idea. It needs to be automated or semi-automated, though. With a teacher clicking on a "capture data" button, she/he could select (copy to wiki, copy to glossary, copy to database) and the data would be moved into a pre-existing or new wiki/glossary/database.

            Show
            hinkelman Don Hinkelman added a comment - >> Moodle has other ways to capture information for later use: >> - you could move the discussion to a special "sticky" forum >> - you could capture the data in a wiki, glossary, or database Are these two new features planned for Moodle 2.0? If so, I believe the "pinned topics" request would be handled, if an admin or teacher could redesignate an ordinary forum into a "Sticky" forum. Then be able select the topic(s) which would become "sticky" (fixed at the top of the list of topics). Presumably, if more than one forum topic is selected as sticky, the teacher could fix the order of the sticky topics (first, second, third...). On the data capture feature, that is another great idea. It needs to be automated or semi-automated, though. With a teacher clicking on a "capture data" button, she/he could select (copy to wiki, copy to glossary, copy to database) and the data would be moved into a pre-existing or new wiki/glossary/database.
            Hide
            dougiamas Martin Dougiamas added a comment -

            Actually I was just talking about standard Moodle ... an ordinary forum can have important discussions moved to it from other forums. And for the other stuff: cut and paste, like we do now for http://moodle.org/useful/ -> http://docs.moodle.org/en

            But your ideas are good anyway!

            Show
            dougiamas Martin Dougiamas added a comment - Actually I was just talking about standard Moodle ... an ordinary forum can have important discussions moved to it from other forums. And for the other stuff: cut and paste, like we do now for http://moodle.org/useful/ -> http://docs.moodle.org/en But your ideas are good anyway!
            Hide
            cfulton Charles Fulton added a comment -

            If anyone's still interested in taking this up I've implemented this functionality in a limited way for 2.0. Users with the new mod/forum:pindiscussions capability can "pin" a discussion so that it goes to the top of a forum. Patch here: https://github.com/mackensen/moodle/compare/mdl-372.

            Show
            cfulton Charles Fulton added a comment - If anyone's still interested in taking this up I've implemented this functionality in a limited way for 2.0. Users with the new mod/forum:pindiscussions capability can "pin" a discussion so that it goes to the top of a forum. Patch here: https://github.com/mackensen/moodle/compare/mdl-372 .
            Hide
            stuartrmealor Stuart R Mealor added a comment -

            "If anyone's still interested in taking this up"...
            I think people ARE interested in it - it has almost 30 votes and watchers.
            I must admit that I'm somewhat perplexed by the way the 'Suggested feature' works.
            There are a lot of requests here that are many years old.
            I'm not sure how we (as a community) have new features added to Moodle each release, and yet <i>some</i> of the ideas here ("New Feature", Improvement) AND resolution = unresolved ORDER BY votes DESC) are REALLY good but it seems almost random how they are actually addressed ?
            I'd like to see a policy where we tick off at least one Feature Request each month (so that would be 6 per Moodle release).
            It may be that some are too difficult, so in that case we pick 6 of the top 8, and delete the ones that are never going to be done.

            Show
            stuartrmealor Stuart R Mealor added a comment - "If anyone's still interested in taking this up"... I think people ARE interested in it - it has almost 30 votes and watchers. I must admit that I'm somewhat perplexed by the way the 'Suggested feature' works. There are a lot of requests here that are many years old. I'm not sure how we (as a community) have new features added to Moodle each release, and yet <i>some</i> of the ideas here ("New Feature", Improvement) AND resolution = unresolved ORDER BY votes DESC) are REALLY good but it seems almost random how they are actually addressed ? I'd like to see a policy where we tick off at least one Feature Request each month (so that would be 6 per Moodle release). It may be that some are too difficult, so in that case we pick 6 of the top 8, and delete the ones that are never going to be done.
            Hide
            hinkelman Don Hinkelman added a comment - - edited

            Thanks, Stuart, for reviving this urgently needed new feature. Yes, sticky forums/pinned topics have been batted around for ages, even before 2006 when this issue was first listed. I think it deserves a push again, because the core of Moodle has always been the forum,and Moodle has the best forums of any LMS. I am really proud of that and it is actually the reason I chose Moodle in 2003.

            The reason we need pinned topics for an admin/teacher is because it aids in knowledge creation. Forums often suffer from an meandering, petering-out syndrome. That is OK in some cases, but sometimes the topic is so critical, that periodic summarizing and focusing can help, and literally create structured knowledge. I see two workflows in such a teaching scenario--one top down, one bottom up.

            • Top-down pinned topic/sticky forum: the teacher or discussion moderator selects a main topic and pins it so it stays at the top of the list, and visually appears slightly emphasized (color or shading). As posts are added, the moderator grabs relevant content and summarizes what the group is learning from each other.
            • Bottom-up pinned topic/sticky forum: Any forum that starts out in a normal, serial pattern, can be switched to a pinned topic mode. A person or persons can be selected or volunteer to summarize the discussion and keep that summary at the top of the discussion. Also, within a large forum, multiple pinned topics can be selected to rise to the top, either by moderator-choice or participant-voting. And of course, a pinned topic could serve as a directory within a large forum, where the key topics can be listed and visitors pointed towards them.

            Another reason we need pinned topics is because knowledge creation is extending beyond a single semester-single co-hort model. As we create learning communities that exist with ongoing purposes beyond merely granting credits for passing a course, we need to keep forums active and growing for many years, even after students graduate. Ways to capture the knowledge can include wiki, but a pinned topic is more organic, and can flow from the discussion. Moodle.org would greatly benefit from this, and give more specific roles for volunteers to help moderate some forums.

            Show
            hinkelman Don Hinkelman added a comment - - edited Thanks, Stuart, for reviving this urgently needed new feature. Yes, sticky forums/pinned topics have been batted around for ages, even before 2006 when this issue was first listed. I think it deserves a push again, because the core of Moodle has always been the forum,and Moodle has the best forums of any LMS. I am really proud of that and it is actually the reason I chose Moodle in 2003. The reason we need pinned topics for an admin/teacher is because it aids in knowledge creation. Forums often suffer from an meandering, petering-out syndrome. That is OK in some cases, but sometimes the topic is so critical, that periodic summarizing and focusing can help, and literally create structured knowledge. I see two workflows in such a teaching scenario--one top down, one bottom up. Top-down pinned topic/sticky forum: the teacher or discussion moderator selects a main topic and pins it so it stays at the top of the list, and visually appears slightly emphasized (color or shading). As posts are added, the moderator grabs relevant content and summarizes what the group is learning from each other. Bottom-up pinned topic/sticky forum: Any forum that starts out in a normal, serial pattern, can be switched to a pinned topic mode. A person or persons can be selected or volunteer to summarize the discussion and keep that summary at the top of the discussion. Also, within a large forum, multiple pinned topics can be selected to rise to the top, either by moderator-choice or participant-voting. And of course, a pinned topic could serve as a directory within a large forum, where the key topics can be listed and visitors pointed towards them. Another reason we need pinned topics is because knowledge creation is extending beyond a single semester-single co-hort model. As we create learning communities that exist with ongoing purposes beyond merely granting credits for passing a course, we need to keep forums active and growing for many years, even after students graduate. Ways to capture the knowledge can include wiki, but a pinned topic is more organic, and can flow from the discussion. Moodle.org would greatly benefit from this, and give more specific roles for volunteers to help moderate some forums.
            Hide
            andreabix Andrea Bicciolo added a comment -

            There are similar requests from MDL-14938. Maybe the issues could be linked.

            Show
            andreabix Andrea Bicciolo added a comment - There are similar requests from MDL-14938 . Maybe the issues could be linked.
            Hide
            cfulton Charles Fulton added a comment -

            I've resurrected my old patch for 2.3. The only outstanding issue is that I can't make the Pin/Unpin button align properly.

            Show
            cfulton Charles Fulton added a comment - I've resurrected my old patch for 2.3. The only outstanding issue is that I can't make the Pin/Unpin button align properly.
            Hide
            lazydaisy Mary Evans added a comment -

            Hi Charles this works great! I've uploaded image to show that in Afterburner I floated it to LEFT and looks OK...would be nice to have a little icon instead, but for now looks to be working as expected.

            Thanks

            Show
            lazydaisy Mary Evans added a comment - Hi Charles this works great! I've uploaded image to show that in Afterburner I floated it to LEFT and looks OK...would be nice to have a little icon instead, but for now looks to be working as expected. Thanks
            Hide
            lazydaisy Mary Evans added a comment -

            Is there any chance to add a class selector to the pinned topic starter something like

            <td class="topic starter pinned">

            so that a small 'pin' icon could be added via CSS in the theme setting? See image above.

            Show
            lazydaisy Mary Evans added a comment - Is there any chance to add a class selector to the pinned topic starter something like <td class="topic starter pinned"> so that a small 'pin' icon could be added via CSS in the theme setting? See image above.
            Hide
            lazydaisy Mary Evans added a comment -

            @Charles

            When you set this for Peer Review, are you not supposed to ask someone to do this then add them as Peer Reviewer?

            If you want to you could add me as Peer Reviewer, however, if it were me I would submit it for Integration Review.

            I personally think this is a great addition to the forum.

            Thanks
            Mary

            Show
            lazydaisy Mary Evans added a comment - @Charles When you set this for Peer Review, are you not supposed to ask someone to do this then add them as Peer Reviewer? If you want to you could add me as Peer Reviewer, however, if it were me I would submit it for Integration Review. I personally think this is a great addition to the forum. Thanks Mary
            Hide
            lazydaisy Mary Evans added a comment - - edited

            @Helen

            I have just added you as a watcher on MDL-372 - Pinned forum topic

            This would be a great addition for the new forum!

            Any chance you could get this added

            Show
            lazydaisy Mary Evans added a comment - - edited @Helen I have just added you as a watcher on MDL-372 - Pinned forum topic This would be a great addition for the new forum! Any chance you could get this added
            Hide
            tsala Helen Foster added a comment -

            I agree that this would be a great feature to have in Moodle and have voted accordingly.

            Charles, thanks for your patch.

            Show
            tsala Helen Foster added a comment - I agree that this would be a great feature to have in Moodle and have voted accordingly. Charles, thanks for your patch.
            Hide
            tsala Helen Foster added a comment -

            Linking to MDL-14938 as suggested by Andrea.

            Show
            tsala Helen Foster added a comment - Linking to MDL-14938 as suggested by Andrea.
            Hide
            drex Mark Drechsler added a comment -

            131 sleeps until this Tracker item will be ten years old - surely that has got to be some kind of record?

            Seriously though, should this also be linked to http://tracker.moodle.org/browse/MDL-21538 on the assumption that this will resolve this one?

            Show
            drex Mark Drechsler added a comment - 131 sleeps until this Tracker item will be ten years old - surely that has got to be some kind of record? Seriously though, should this also be linked to http://tracker.moodle.org/browse/MDL-21538 on the assumption that this will resolve this one?
            Hide
            gb2048 Gareth J Barnard added a comment -

            This would be a useful feature to have on the Moodle forums for Moderators to pin temporarily popular issues / information to the top.

            I think that MDL-8 might be the oldest open tracker issue

            Show
            gb2048 Gareth J Barnard added a comment - This would be a useful feature to have on the Moodle forums for Moderators to pin temporarily popular issues / information to the top. I think that MDL-8 might be the oldest open tracker issue
            Hide
            gb2048 Gareth J Barnard added a comment -

            Hi,

            Is this ready for 'Peer Review'?

            Cheers,

            Gareth

            Show
            gb2048 Gareth J Barnard added a comment - Hi, Is this ready for 'Peer Review'? Cheers, Gareth
            Hide
            trogdor Julian Ridden added a comment -

            Was there not work being done on getting ForumNG into core? When that finally lands it does allow teachers to Pin posts
            Julian

            Show
            trogdor Julian Ridden added a comment - Was there not work being done on getting ForumNG into core? When that finally lands it does allow teachers to Pin posts Julian
            Hide
            derekcx Derek Chirnside added a comment -

            @Julian. ForumNG in core in the tracker: MDL-21538

            I suspect this is dead in the water now. This is the latest conversation: https://tracker.moodle.org/browse/MDL-39707 looking at porting Advanced Forums features into Moodle. Martin has recently posted here: https://tracker.moodle.org/browse/MDLSITE-1910

            Forums are a complex issue.

            -Derek

            Show
            derekcx Derek Chirnside added a comment - @Julian. ForumNG in core in the tracker: MDL-21538 I suspect this is dead in the water now. This is the latest conversation: https://tracker.moodle.org/browse/MDL-39707 looking at porting Advanced Forums features into Moodle. Martin has recently posted here: https://tracker.moodle.org/browse/MDLSITE-1910 Forums are a complex issue. -Derek
            Hide
            ozjuliancox Julian Cox added a comment -

            Really like the idea, but why limit access to admins and instructors? Could this be available to students and why not link pinning to rating? This way, students could bring to the top the topics or posts that most interest them.

            Show
            ozjuliancox Julian Cox added a comment - Really like the idea, but why limit access to admins and instructors? Could this be available to students and why not link pinning to rating? This way, students could bring to the top the topics or posts that most interest them.
            Hide
            lazydaisy Mary Evans added a comment -

            Not a good idea to give students that ability, as all this would do, Julian, is end up with the top 1000 posts stuck at the top of the forum, so you would be scrollllllllling forever!

            A better alternative for students would be a thumbs up icon added to the discussion title that they liked. This is what happens in phpBB forums. Indeed I wish Moodle would adopt some of the ideas. From phpBB open source software, as they have had 'sticky topic' function that adds a pin to the post that sits at the top of the main posts.

            Show
            lazydaisy Mary Evans added a comment - Not a good idea to give students that ability, as all this would do, Julian, is end up with the top 1000 posts stuck at the top of the forum, so you would be scrollllllllling forever! A better alternative for students would be a thumbs up icon added to the discussion title that they liked. This is what happens in phpBB forums. Indeed I wish Moodle would adopt some of the ideas. From phpBB open source software, as they have had 'sticky topic' function that adds a pin to the post that sits at the top of the main posts.
            Hide
            marina Marina Glancy added a comment -

            I can see that in the proposed patch there is a new capability to pin discussions. This means that each institution (or even each teacher in the course) can decide for themselves who is allowed to do it. So we don't need to argue whether students can do it or not.

            Show
            marina Marina Glancy added a comment - I can see that in the proposed patch there is a new capability to pin discussions. This means that each institution (or even each teacher in the course) can decide for themselves who is allowed to do it. So we don't need to argue whether students can do it or not.
            Hide
            cfulton Charles Fulton added a comment -

            Marina Glancy: I think I based that patch off 2.3; I'll see if I can get it refreshed today.

            Show
            cfulton Charles Fulton added a comment - Marina Glancy : I think I based that patch off 2.3; I'll see if I can get it refreshed today.
            Hide
            cfulton Charles Fulton added a comment -

            I've refreshed the patch against 2.9dev, which mostly involved writing new events. I haven't solved the issue of getting the "pin/unpin" button to display properly. That's a blocker for integration.

            Show
            cfulton Charles Fulton added a comment - I've refreshed the patch against 2.9dev, which mostly involved writing new events. I haven't solved the issue of getting the "pin/unpin" button to display properly. That's a blocker for integration.
            Hide
            marina Marina Glancy added a comment -

            This looks really good Charles, thanks!

            Show
            marina Marina Glancy added a comment - This looks really good Charles, thanks!
            Hide
            derekcx Derek Chirnside added a comment -

            Will this make it into 2.9?
            Here's hoping.

            -Derek

            Show
            derekcx Derek Chirnside added a comment - Will this make it into 2.9? Here's hoping. -Derek
            Hide
            berserkk Ben Kelada added a comment -

            Updated to moodle 3.0 master,
            fixed alignment of pin button
            added pin image instead of "pinned" text

            Show
            berserkk Ben Kelada added a comment - Updated to moodle 3.0 master, fixed alignment of pin button added pin image instead of "pinned" text
            Hide
            cibot CiBoT added a comment -

            Fails against automated checks.

            Checked MDL-372 using repository: git://github.com/BenKelada/moodle.git

            More information about this report

            Show
            cibot CiBoT added a comment - Fails against automated checks. Checked MDL-372 using repository: git://github.com/BenKelada/moodle.git master (16 errors / 0 warnings) [branch: MDL-372-master | CI Job ] phplint (0/0) , php (16/0) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , More information about this report
            Hide
            lazydaisy Mary Evans added a comment -
            Show
            lazydaisy Mary Evans added a comment - You will need to fix those 16 errors first. http://integration.moodle.org/job/Precheck%20remote%20branch/18317/artifact/work/smurf.html#php
            Hide
            berserkk Ben Kelada added a comment -

            Mary Evans I did look at those, wasn't sure i should leave the whitespace consistent with the previous code or fix it all.
            I've pushed a version fixing all the whitespace/codesniffer items in the file

            Show
            berserkk Ben Kelada added a comment - Mary Evans I did look at those, wasn't sure i should leave the whitespace consistent with the previous code or fix it all. I've pushed a version fixing all the whitespace/codesniffer items in the file
            Hide
            cibot CiBoT added a comment -

            Code verified against automated checks.

            Checked MDL-372 using repository: git://github.com/BenKelada/moodle.git

            More information about this report

            Show
            cibot CiBoT added a comment - Code verified against automated checks. Checked MDL-372 using repository: git://github.com/BenKelada/moodle.git master (0 errors / 0 warnings) [branch: MDL-372-master | CI Job ] More information about this report
            Hide
            cibot CiBoT added a comment -

            Fails against automated checks.

            Checked MDL-372 using repository: git://github.com/BenKelada/moodle.git

            • master [branch: MDL-372-master | CI Job]
              • Info: the branch is based off moodle.git
              • Info: base commit 2b11b94c0ca3b8c7b352fd9abdc90b7fd21da0cd being used as initial commit.
              • Error: The MDL-372-master branch at git://github.com/BenKelada/moodle.git does not apply clean to origin/master

            More information about this report

            Show
            cibot CiBoT added a comment - Fails against automated checks. Checked MDL-372 using repository: git://github.com/BenKelada/moodle.git master [branch: MDL-372-master | CI Job ] Info: the branch is based off moodle.git Info: base commit 2b11b94c0ca3b8c7b352fd9abdc90b7fd21da0cd being used as initial commit. Error: The MDL-372 -master branch at git://github.com/BenKelada/moodle.git does not apply clean to origin/master More information about this report
            Hide
            cibot CiBoT added a comment -

            Code verified against automated checks.

            Checked MDL-372 using repository: git://github.com/BenKelada/moodle.git

            More information about this report

            Show
            cibot CiBoT added a comment - Code verified against automated checks. Checked MDL-372 using repository: git://github.com/BenKelada/moodle.git master (0 errors / 0 warnings) [branch: MDL-372-master | CI Job ] More information about this report
            Hide
            berserkk Ben Kelada added a comment -

            added pin option to start new discussion form

            Show
            berserkk Ben Kelada added a comment - added pin option to start new discussion form
            Hide
            cibot CiBoT added a comment -

            Code verified against automated checks.

            Checked MDL-372 using repository: git://github.com/BenKelada/moodle.git

            More information about this report

            Show
            cibot CiBoT added a comment - Code verified against automated checks. Checked MDL-372 using repository: git://github.com/BenKelada/moodle.git master (0 errors / 0 warnings) [branch: MDL-372-master | CI Job ] More information about this report
            Hide
            cibot CiBoT added a comment -

            Fails against automated checks.

            Checked MDL-372 using repository: git://github.com/BenKelada/moodle.git

            More information about this report

            Show
            cibot CiBoT added a comment - Fails against automated checks. Checked MDL-372 using repository: git://github.com/BenKelada/moodle.git master (6 errors / 0 warnings) [branch: MDL-372-master | CI Job ] phplint (0/0) , php (6/0) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , More information about this report
            Hide
            cibot CiBoT added a comment -

            Fails against automated checks.

            Checked MDL-372 using repository: git://github.com/BenKelada/moodle.git

            More information about this report

            Show
            cibot CiBoT added a comment - Fails against automated checks. Checked MDL-372 using repository: git://github.com/BenKelada/moodle.git master (1 errors / 0 warnings) [branch: MDL-372-master | CI Job ] phplint (0/0) , php (1/0) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , More information about this report
            Hide
            cibot CiBoT added a comment -

            Code verified against automated checks.

            Checked MDL-372 using repository: git://github.com/BenKelada/moodle.git

            More information about this report

            Show
            cibot CiBoT added a comment - Code verified against automated checks. Checked MDL-372 using repository: git://github.com/BenKelada/moodle.git master (0 errors / 0 warnings) [branch: MDL-372-master | CI Job ] More information about this report
            Hide
            berserkk Ben Kelada added a comment -

            added pinned options to webservices (add discussion, get discussions paginated)

            Show
            berserkk Ben Kelada added a comment - added pinned options to webservices (add discussion, get discussions paginated)
            Hide
            cibot CiBoT added a comment -

            Code verified against automated checks.

            Checked MDL-372 using repository: git://github.com/BenKelada/moodle.git

            More information about this report

            Show
            cibot CiBoT added a comment - Code verified against automated checks. Checked MDL-372 using repository: git://github.com/BenKelada/moodle.git master (0 errors / 0 warnings) [branch: MDL-372-master | CI Job ] More information about this report
            Hide
            berserkk Ben Kelada added a comment -

            fixed broken test, sorry for all the spam

            Show
            berserkk Ben Kelada added a comment - fixed broken test, sorry for all the spam
            Hide
            cibot CiBoT added a comment -

            Code verified against automated checks.

            Checked MDL-372 using repository: git://github.com/BenKelada/moodle.git

            More information about this report

            Show
            cibot CiBoT added a comment - Code verified against automated checks. Checked MDL-372 using repository: git://github.com/BenKelada/moodle.git master (0 errors / 0 warnings) [branch: MDL-372-master | CI Job ] More information about this report
            Hide
            berserkk Ben Kelada added a comment -

            updated from review
            Added support for "neighbour" function
            other various fixes as well

            Show
            berserkk Ben Kelada added a comment - updated from review Added support for "neighbour" function other various fixes as well
            Hide
            cibot CiBoT added a comment -

            Fails against automated checks.

            Checked MDL-372 using repository: git://github.com/BenKelada/moodle.git

            More information about this report

            Show
            cibot CiBoT added a comment - Fails against automated checks. Checked MDL-372 using repository: git://github.com/BenKelada/moodle.git master (8 errors / 5 warnings) [branch: MDL-372-master | CI Job ] phplint (0/0) , php (6/5) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (2/0) , savepoint (0/0) , thirdparty (0/0) , grunt (0/0) , More information about this report
            Hide
            cibot CiBoT added a comment -

            Fails against automated checks.

            Checked MDL-372 using repository: git://github.com/BenKelada/moodle.git

            More information about this report

            Show
            cibot CiBoT added a comment - Fails against automated checks. Checked MDL-372 using repository: git://github.com/BenKelada/moodle.git master (2 errors / 5 warnings) [branch: MDL-372-master | CI Job ] phplint (0/0) , php (2/5) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , grunt (0/0) , More information about this report
            Hide
            cibot CiBoT added a comment -

            Code verified against automated checks.

            Checked MDL-372 using repository: git://github.com/BenKelada/moodle.git

            More information about this report

            Show
            cibot CiBoT added a comment - Code verified against automated checks. Checked MDL-372 using repository: git://github.com/BenKelada/moodle.git master (0 errors / 0 warnings) [branch: MDL-372-master | CI Job ] More information about this report
            Hide
            ryanwyllie Ryan Wyllie added a comment -

            Hey Ben,

            Thanks a lot for working on this issue, it looks like it's been a long requested feature and is definitely a tricky one to implement. Overall I think your patch looks pretty great and is very comprehensive. After looking over your change I've just got a few points for you to consider (sorry if the list is long, it's a large patch, haha):

            1. Should the 'mod/forum:pindiscussions' capability be of type “write” since it allows you to modify the forum post data?
            2. I’m not 100% convinced by this change which automatically prefixes “d.pinned DESC” to all custom sorting requests. It means you can never use that API to retrieve discussions in the order you choose, making it fairly inflexible. I think it would be good to have the default ordering using the pinned column but I’m not so sure about making it mandatory on all custom sorting as well. There may be some cases where you might not want pinned discussions to take priority (maybe in a blog style forum?). As a side note, I suspect this is also going to cause problems when this function is called with $sort being the value returned from forum_get_default_sort_order(), such as what forum_print_latest_discussions() does, where the SQL statement will end up with 2 “d.pinned DESC” commands in the ORDER BY clause (one from forum_get_default_sort_order() and one from the hardcoded prefix in this line). I’m not sure if that will cause an issue with some databases.
            3. Again adding some mandatory ordering with this change, which may not be generally applicable. If you wanted to add that change I think it should ideally be in the forum_get_default_sort_order() so that it isn’t mandatory (inline with my point above). If you did want to make it mandatory I think adding it to this line would be the best place, since that’s where the other mandatory sorting has been hardcoded in.
            4. You’ve made a lot of changes to forum_get_discussion_neighbours() which I’m a little bit weary of because of how crazy complex that function is, haha. Some things I’ve notice with your changes to it:
              1. You’re now doing a join with for forum_posts table in all cases, when it isn’t used by anything other than the blog forums. That’s going to slow the query down unnecessarily. It would probably be best to only do the join when it’s required.
              2. In this and this line you’re sorting by id but haven’t included the table prefix, it should probably be d.id? Also for the $nextsql should it be “id ASC” instead of “id DESC”?
              3. On those 2 lines above you’re also doing a boolean expression in the order by clause (d.pinned = :pinnedstate3), does that work across multiple databases? The other option is to use the case statement to achieve the same thing (CASE WHEN d.pinned = :pinnedstate3 THEN 1 ELSE 0) which we know is compatible (since we’re already using one).
              4. You’ve added in sorting by id which was explicitly ignored by the first implementation of this function. I think it’s actually safe in this case because you’re only using it as a fall back on identical timestamped discussions. I’m more pointing this out for Andrew’s benefit to have a think about.
            5. Please update the testing instructions
              1. Mention the need to test on multiple databases (MySQL, Postgres, Oracle etc)
              2. Add more testing instructions to cover some complex scenarios having different combinations of pinned, time delayed (e.g. starting in the future) and normal posts forums posts.
              3. Add some testing instructions for different types of forums (regular, blog etc)
              4. Add a scenario to cover backup / restore of a course with pinned discussions
            6. Thanks a lot for updating the unit tests with new scenarios covering the pinned discussions. It would be really great to add some behat tests to cover off the front end for pinning discussions, making sure the button is visible, the issue rises to the top etc.

            I also wanted to ping Andrew Nicols to have a quick look over this when he's got a spare second because he knows everything there is to know about all things and is especially well versed in the Moodle forum code.

            Show
            ryanwyllie Ryan Wyllie added a comment - Hey Ben, Thanks a lot for working on this issue, it looks like it's been a long requested feature and is definitely a tricky one to implement. Overall I think your patch looks pretty great and is very comprehensive. After looking over your change I've just got a few points for you to consider (sorry if the list is long, it's a large patch, haha): Should the 'mod/forum:pindiscussions' capability be of type “write” since it allows you to modify the forum post data? I’m not 100% convinced by this change which automatically prefixes “d.pinned DESC” to all custom sorting requests. It means you can never use that API to retrieve discussions in the order you choose, making it fairly inflexible. I think it would be good to have the default ordering using the pinned column but I’m not so sure about making it mandatory on all custom sorting as well. There may be some cases where you might not want pinned discussions to take priority (maybe in a blog style forum?). As a side note, I suspect this is also going to cause problems when this function is called with $sort being the value returned from forum_get_default_sort_order(), such as what forum_print_latest_discussions() does, where the SQL statement will end up with 2 “d.pinned DESC” commands in the ORDER BY clause (one from forum_get_default_sort_order() and one from the hardcoded prefix in this line). I’m not sure if that will cause an issue with some databases. Again adding some mandatory ordering with this change, which may not be generally applicable. If you wanted to add that change I think it should ideally be in the forum_get_default_sort_order() so that it isn’t mandatory (inline with my point above). If you did want to make it mandatory I think adding it to this line would be the best place, since that’s where the other mandatory sorting has been hardcoded in. You’ve made a lot of changes to forum_get_discussion_neighbours() which I’m a little bit weary of because of how crazy complex that function is, haha. Some things I’ve notice with your changes to it: You’re now doing a join with for forum_posts table in all cases, when it isn’t used by anything other than the blog forums. That’s going to slow the query down unnecessarily. It would probably be best to only do the join when it’s required. In this and this line you’re sorting by id but haven’t included the table prefix, it should probably be d.id? Also for the $nextsql should it be “id ASC” instead of “id DESC”? On those 2 lines above you’re also doing a boolean expression in the order by clause (d.pinned = :pinnedstate3), does that work across multiple databases? The other option is to use the case statement to achieve the same thing (CASE WHEN d.pinned = :pinnedstate3 THEN 1 ELSE 0) which we know is compatible (since we’re already using one). You’ve added in sorting by id which was explicitly ignored by the first implementation of this function. I think it’s actually safe in this case because you’re only using it as a fall back on identical timestamped discussions. I’m more pointing this out for Andrew’s benefit to have a think about. Please update the testing instructions Mention the need to test on multiple databases (MySQL, Postgres, Oracle etc) Add more testing instructions to cover some complex scenarios having different combinations of pinned, time delayed (e.g. starting in the future) and normal posts forums posts. Add some testing instructions for different types of forums (regular, blog etc) Add a scenario to cover backup / restore of a course with pinned discussions Thanks a lot for updating the unit tests with new scenarios covering the pinned discussions. It would be really great to add some behat tests to cover off the front end for pinning discussions, making sure the button is visible, the issue rises to the top etc. I also wanted to ping Andrew Nicols to have a quick look over this when he's got a spare second because he knows everything there is to know about all things and is especially well versed in the Moodle forum code.
            Hide
            berserkk Ben Kelada added a comment - - edited

            Thanks for reviewing Ryan!
            1) yes should be write, will patch that
            2) -the sorting ignoring pinned is gets a bit tricky, maybe i could add a parameter to the forum_get_discussions $sortpinned = 1 , - updated this
            3) the forced sort by id is really just a catch all to make sure ordering is consistent for whatever sort order is chosen , removed the other hard coded sort
            4) i) i thought it would be a little simpler(?!) having the one query, it should really be a full join, and its only on the discussion->1 post, so it shouldnt be a performance issue as a full join. I can also split the query and make it blog only if required,
            ii) will fix, I intentionally left them both DESC, to force a consistent order, changed as it is less confusing
            iii) not sure? should i change it just in case?
            iv) yeah thats what i figured, it only forces an order when time modified is the same

            5) can do.

            6) if anyone can assist with the behat tests that would be great, i need to setup my environment again, and wont have time for a bit

            Will update the fixes in a few days, if not shorter, would be good to hear andrews thoughts too
            cheers,
            Ben

            Show
            berserkk Ben Kelada added a comment - - edited Thanks for reviewing Ryan! 1) yes should be write, will patch that 2) -the sorting ignoring pinned is gets a bit tricky, maybe i could add a parameter to the forum_get_discussions $sortpinned = 1 , - updated this 3) the forced sort by id is really just a catch all to make sure ordering is consistent for whatever sort order is chosen , removed the other hard coded sort 4) i) i thought it would be a little simpler(?!) having the one query, it should really be a full join, and its only on the discussion->1 post, so it shouldnt be a performance issue as a full join. I can also split the query and make it blog only if required, ii) will fix, I intentionally left them both DESC, to force a consistent order, changed as it is less confusing iii) not sure? should i change it just in case? iv) yeah thats what i figured, it only forces an order when time modified is the same 5) can do. 6) if anyone can assist with the behat tests that would be great, i need to setup my environment again, and wont have time for a bit Will update the fixes in a few days, if not shorter, would be good to hear andrews thoughts too cheers, Ben
            Hide
            cibot CiBoT added a comment -

            Fails against automated checks.

            Checked MDL-372 using repository: git://github.com/BenKelada/moodle.git

            • master [branch: MDL-372-master | CI Job]
              • Info: the branch is based off moodle.git
              • Info: base commit 0dfcc2541a61e8e8ee6c29e262de76312e1bcbc2 being used as initial commit.
              • Error: The MDL-372-master branch at git://github.com/BenKelada/moodle.git does not apply clean to origin/master

            More information about this report

            Show
            cibot CiBoT added a comment - Fails against automated checks. Checked MDL-372 using repository: git://github.com/BenKelada/moodle.git master [branch: MDL-372-master | CI Job ] Info: the branch is based off moodle.git Info: base commit 0dfcc2541a61e8e8ee6c29e262de76312e1bcbc2 being used as initial commit. Error: The MDL-372 -master branch at git://github.com/BenKelada/moodle.git does not apply clean to origin/master More information about this report
            Hide
            cibot CiBoT added a comment -

            Fails against automated checks.

            Checked MDL-372 using repository: git://github.com/BenKelada/moodle.git

            More information about this report

            Show
            cibot CiBoT added a comment - Fails against automated checks. Checked MDL-372 using repository: git://github.com/BenKelada/moodle.git master (4 errors / 0 warnings) [branch: MDL-372-master | CI Job ] phplint (0/0) , php (0/0) , js (0/0) , css (0/0) , phpdoc (4/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , grunt (0/0) , More information about this report
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Pinging Ryan Wyllie - let's try and pick this up after the new year. I think that this will resolve MDL-52349.

            Show
            dobedobedoh Andrew Nicols added a comment - Pinging Ryan Wyllie - let's try and pick this up after the new year. I think that this will resolve MDL-52349 .
            Hide
            ryanwyllie Ryan Wyllie added a comment -

            Just taking another look at this now and it seems like Ben has made the changes we discussed (thanks!). I'll have a go at adding a few behat tests before moving this on to integration.

            Show
            ryanwyllie Ryan Wyllie added a comment - Just taking another look at this now and it seems like Ben has made the changes we discussed (thanks!). I'll have a go at adding a few behat tests before moving this on to integration.
            Hide
            ryanwyllie Ryan Wyllie added a comment -

            After discussing it with Andrew we agreed that behat tests would be less useful in this case than expanding the unit test coverage. So I wrote a few more unit tests for this code to cover off pinned discussions with groups, with timed posts and pinned timed posts.

            I found a small bug with the SQL statement for getting the neighbours that was causing it to return a value when it shouldn't be due to the condition that falls back to searching by id. I've added in a small change to that to make sure that it also takes into consideration the pinned status of the discussion before falling back to the id.

            I also rebased the changes on the last stable master and fixed up all of the conflicts there.

            I think this is now good to go to integration.

            Thanks again for all of your work on this, Ben.

            Show
            ryanwyllie Ryan Wyllie added a comment - After discussing it with Andrew we agreed that behat tests would be less useful in this case than expanding the unit test coverage. So I wrote a few more unit tests for this code to cover off pinned discussions with groups, with timed posts and pinned timed posts. I found a small bug with the SQL statement for getting the neighbours that was causing it to return a value when it shouldn't be due to the condition that falls back to searching by id. I've added in a small change to that to make sure that it also takes into consideration the pinned status of the discussion before falling back to the id. I also rebased the changes on the last stable master and fixed up all of the conflicts there. I think this is now good to go to integration. Thanks again for all of your work on this, Ben.
            Hide
            cibot CiBoT added a comment -

            Fails against automated checks.

            Checked MDL-372 using repository: git://github.com/ryanwyllie/moodle.git

            More information about this report

            Show
            cibot CiBoT added a comment - Fails against automated checks. Checked MDL-372 using repository: git://github.com/ryanwyllie/moodle.git master (4 errors / 29 warnings) [branch: MDL-372-master | CI Job ] phplint (0/0) , php (0/29) , js (0/0) , css (0/0) , phpdoc (4/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , grunt (0/0) , shifter (0/0) , More information about this report
            Hide
            ryanwyllie Ryan Wyllie added a comment -

            Whoops, looks like I need to fix up a couple of CiBoT errors. I also realised that I need to fix up some of the existing behat tests that do the "wait for 1 seconds" step, since we shouldn't need that now that we've got ordering by id.

            Show
            ryanwyllie Ryan Wyllie added a comment - Whoops, looks like I need to fix up a couple of CiBoT errors. I also realised that I need to fix up some of the existing behat tests that do the "wait for 1 seconds" step, since we shouldn't need that now that we've got ordering by id.
            Hide
            cibot CiBoT added a comment -

            Fails against automated checks.

            Checked MDL-372 using repository: git://github.com/ryanwyllie/moodle.git

            • master [branch: MDL-372-master | CI Job]
              • Info: the branch is based off moodle.git
              • Info: base commit e65dfd9f288725eaf07b8273db79a62aa8e12f3c being used as initial commit.
              • Error: The MDL-372-master branch at git://github.com/ryanwyllie/moodle.git does not apply clean to origin/master

            More information about this report

            Show
            cibot CiBoT added a comment - Fails against automated checks. Checked MDL-372 using repository: git://github.com/ryanwyllie/moodle.git master [branch: MDL-372-master | CI Job ] Info: the branch is based off moodle.git Info: base commit e65dfd9f288725eaf07b8273db79a62aa8e12f3c being used as initial commit. Error: The MDL-372 -master branch at git://github.com/ryanwyllie/moodle.git does not apply clean to origin/master More information about this report
            Hide
            ryanwyllie Ryan Wyllie added a comment -

            Ok, fixed up the warnings and the errors. Also removed the "wait 1 seconds" steps from the behat tests and ran them to confirm they still pass. This should be all good now.

            Show
            ryanwyllie Ryan Wyllie added a comment - Ok, fixed up the warnings and the errors. Also removed the "wait 1 seconds" steps from the behat tests and ran them to confirm they still pass. This should be all good now.
            Hide
            cibot CiBoT added a comment -

            Code verified against automated checks with warnings.

            Checked MDL-372 using repository: git://github.com/ryanwyllie/moodle.git

            More information about this report

            Show
            cibot CiBoT added a comment - Code verified against automated checks with warnings. Checked MDL-372 using repository: git://github.com/ryanwyllie/moodle.git master (0 errors / 1 warnings) [branch: MDL-372-master | CI Job ] phplint (0/0) , php (0/1) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , grunt (0/0) , shifter (0/0) , More information about this report
            Hide
            cibot CiBoT added a comment -

            Code verified against automated checks.

            Checked MDL-372 using repository: git://github.com/ryanwyllie/moodle.git

            More information about this report

            Show
            cibot CiBoT added a comment - Code verified against automated checks. Checked MDL-372 using repository: git://github.com/ryanwyllie/moodle.git master (0 errors / 0 warnings) [branch: MDL-372-master | CI Job ] More information about this report
            Hide
            cibot CiBoT added a comment -

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

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

            Code verified against automated checks.

            Checked MDL-372 using repository: git://github.com/ryanwyllie/moodle.git

            More information about this report

            Show
            cibot CiBoT added a comment - Code verified against automated checks. Checked MDL-372 using repository: git://github.com/ryanwyllie/moodle.git master (0 errors / 0 warnings) [branch: MDL-372-master | CI Job ] More information about this report
            Hide
            poltawski Dan Poltawski added a comment -

            Hi Ryan,

            This is currently failing on mysql:
            https://travis-ci.org/ryanwyllie/moodle/jobs/100972029

            Show
            poltawski Dan Poltawski added a comment - Hi Ryan, This is currently failing on mysql: https://travis-ci.org/ryanwyllie/moodle/jobs/100972029
            Hide
            ryanwyllie Ryan Wyllie added a comment -

            Thanks, Dan. I must've missed the "::integer" casting in my review. I've removed the casting in the SQL statement and done it in the code instead. The build should be ok now.

            Show
            ryanwyllie Ryan Wyllie added a comment - Thanks, Dan. I must've missed the "::integer" casting in my review. I've removed the casting in the SQL statement and done it in the code instead. The build should be ok now.
            Hide
            dobedobedoh Andrew Nicols added a comment -
            Show
            dobedobedoh Andrew Nicols added a comment - Pinging Barbara Ramiro - can you take a look at the png/svg in this branch: https://github.com/ryanwyllie/moodle/compare/e65dfd9...MDL-372-master#diff-b1fdddd9ad399c39c6bd06a9bb1646ac Cheers, Andrew
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Hi Ben + Ryan,

            Thanks for getting this issue together after all this time. I've been following it in the background for a few weeks now.

            There are a few minor issues which need to be resolved before this can be integrated:

            1. https://github.com/ryanwyllie/moodle/compare/e65dfd9...ba697260dab96ec01de77d2c9bc6419c0b5201a8#diff-93d5236b32d211736b4a0ac4cafb7028: The version number was not updated, which suggests that this entry was hand-written. Please can you use XMLDB editor to ensure that it's correct, etc. and that the version number is bumped.
            2. https://github.com/ryanwyllie/moodle/compare/e65dfd9...ba697260dab96ec01de77d2c9bc6419c0b5201a8#diff-b214ea402c9798e063064078588f93d3R726 - it seems that we are doing a get_record() to fetch a single field here. It's not a major overhead, but it's completely avoidable. The best option would be to simply unset($fromform->pinned); here, and stop depending upon it in forum_update_discussion() - e.g. add an isset test there.
            3. https://github.com/ryanwyllie/moodle/compare/e65dfd9...ba697260dab96ec01de77d2c9bc6419c0b5201a8#diff-b214ea402c9798e063064078588f93d3R875 - two lines of whitespace
            4. https://github.com/ryanwyllie/moodle/compare/e65dfd9...ba697260dab96ec01de77d2c9bc6419c0b5201a8#diff-e25e90247fb19916c11517d9315bc713 - not sure what this behat change has to do with this issue
            5. https://github.com/ryanwyllie/moodle/compare/e65dfd9...ba697260dab96ec01de77d2c9bc6419c0b5201a8#diff-bf08c1a576812f8fe4a810a5a2c20cc7 - ditto this one
            6. https://github.com/ryanwyllie/moodle/compare/e65dfd9...ba697260dab96ec01de77d2c9bc6419c0b5201a8#diff-c6df38863603a665ae4ebeb093e0a77d and https://github.com/ryanwyllie/moodle/compare/e65dfd9...ba697260dab96ec01de77d2c9bc6419c0b5201a8#diff-e5919cdd8daf11c35af73d4f004b22a9 should use the FORUM_DISCUSSION_PINNED and friend constants
            7. https://github.com/ryanwyllie/moodle/compare/e65dfd9...ba697260dab96ec01de77d2c9bc6419c0b5201a8#diff-968b624e975ad3df52446c206926d60aR4396 - I'm not necessarily so keen on this change. This value is used for timemodified, and I don't think that we should encourage overriding of the time modified in this fashion. That said, it is only a minor concern. Can I ask why this change was made?

            As I say, these are all minors issues and it should be pretty quick to resolve them.

            If you're unable to get to them before tomorrow morning, could you please ping Ryan so that he may update the issue and we can get this issue integrated.

            Cheers,

            Andrew

            Show
            dobedobedoh Andrew Nicols added a comment - Hi Ben + Ryan, Thanks for getting this issue together after all this time. I've been following it in the background for a few weeks now. There are a few minor issues which need to be resolved before this can be integrated: https://github.com/ryanwyllie/moodle/compare/e65dfd9...ba697260dab96ec01de77d2c9bc6419c0b5201a8#diff-93d5236b32d211736b4a0ac4cafb7028: The version number was not updated, which suggests that this entry was hand-written. Please can you use XMLDB editor to ensure that it's correct, etc. and that the version number is bumped. https://github.com/ryanwyllie/moodle/compare/e65dfd9...ba697260dab96ec01de77d2c9bc6419c0b5201a8#diff-b214ea402c9798e063064078588f93d3R726 - it seems that we are doing a get_record() to fetch a single field here. It's not a major overhead, but it's completely avoidable. The best option would be to simply unset($fromform->pinned); here, and stop depending upon it in forum_update_discussion() - e.g. add an isset test there. https://github.com/ryanwyllie/moodle/compare/e65dfd9...ba697260dab96ec01de77d2c9bc6419c0b5201a8#diff-b214ea402c9798e063064078588f93d3R875 - two lines of whitespace https://github.com/ryanwyllie/moodle/compare/e65dfd9...ba697260dab96ec01de77d2c9bc6419c0b5201a8#diff-e25e90247fb19916c11517d9315bc713 - not sure what this behat change has to do with this issue https://github.com/ryanwyllie/moodle/compare/e65dfd9...ba697260dab96ec01de77d2c9bc6419c0b5201a8#diff-bf08c1a576812f8fe4a810a5a2c20cc7 - ditto this one https://github.com/ryanwyllie/moodle/compare/e65dfd9...ba697260dab96ec01de77d2c9bc6419c0b5201a8#diff-c6df38863603a665ae4ebeb093e0a77d and https://github.com/ryanwyllie/moodle/compare/e65dfd9...ba697260dab96ec01de77d2c9bc6419c0b5201a8#diff-e5919cdd8daf11c35af73d4f004b22a9 should use the FORUM_DISCUSSION_PINNED and friend constants https://github.com/ryanwyllie/moodle/compare/e65dfd9...ba697260dab96ec01de77d2c9bc6419c0b5201a8#diff-968b624e975ad3df52446c206926d60aR4396 - I'm not necessarily so keen on this change. This value is used for timemodified, and I don't think that we should encourage overriding of the time modified in this fashion. That said, it is only a minor concern. Can I ask why this change was made? As I say, these are all minors issues and it should be pretty quick to resolve them. If you're unable to get to them before tomorrow morning, could you please ping Ryan so that he may update the issue and we can get this issue integrated. Cheers, Andrew
            Hide
            berserkk Ben Kelada added a comment -

            I can give a quick comment on 7)
            that change was made to support setting timemodified for phpunit tests, instead of using sleep(1)

            Show
            berserkk Ben Kelada added a comment - I can give a quick comment on 7) that change was made to support setting timemodified for phpunit tests, instead of using sleep(1)
            Hide
            ryanwyllie Ryan Wyllie added a comment -

            Hey Ben,

            I've had a quick look at some of the issues that Andrew raised. I've created a new branch that is rebased on the integration/master branch because it these changes were conflicting (I've now fixed the conflicts). I've also added a couple of commits on top of that to address point 2 raised.

            Feel free to grab this branch, if you'd like. Hopefully it helps. Please let me know if you need a hand with anything.

            git://github.com/ryanwyllie/moodle.git MDL-372-int-master

            Show
            ryanwyllie Ryan Wyllie added a comment - Hey Ben, I've had a quick look at some of the issues that Andrew raised. I've created a new branch that is rebased on the integration/master branch because it these changes were conflicting (I've now fixed the conflicts). I've also added a couple of commits on top of that to address point 2 raised. Feel free to grab this branch, if you'd like. Hopefully it helps. Please let me know if you need a hand with anything. git://github.com/ryanwyllie/moodle.git MDL-372 -int-master
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Thanks for commenting Ben,

            Personally I'm not keen to accept that particular change, but in this interests of getting this issue completed I think we'll allow it. Rajesh Taneja commented similarly during PR of MDL-52597 and I opposed it then too, but perhaps it should be allowed through.

            Do you think you'll be able to make the remaining changes too?

            Cheers,

            Andrew

            Show
            dobedobedoh Andrew Nicols added a comment - Thanks for commenting Ben, Personally I'm not keen to accept that particular change, but in this interests of getting this issue completed I think we'll allow it. Rajesh Taneja commented similarly during PR of MDL-52597 and I opposed it then too, but perhaps it should be allowed through. Do you think you'll be able to make the remaining changes too? Cheers, Andrew
            Hide
            barbararamiro Barbara Ramiro added a comment - - edited

            I had a look at the svg and the png versions. The quality looks good, the svg code looks neat. But the icon is 16x16 which should be in pix/i (icons: 16x16) not pix/t (tiny icons: 12x12) or the location might be right but the image should be resized to 12x12 as it is an action icon if I understood the context of use right. See Dimensions on https://docs.moodle.org/dev/Moodle_icons#Dimensions for which size to use based on context.

            Cheers (" ,)

            Show
            barbararamiro Barbara Ramiro added a comment - - edited I had a look at the svg and the png versions. The quality looks good, the svg code looks neat. But the icon is 16x16 which should be in pix/i (icons: 16x16) not pix/t (tiny icons: 12x12) or the location might be right but the image should be resized to 12x12 as it is an action icon if I understood the context of use right. See Dimensions on https://docs.moodle.org/dev/Moodle_icons#Dimensions for which size to use based on context. Cheers (" ,)
            Hide
            ryanwyllie Ryan Wyllie added a comment -

            Haven't heard back from Ben so I'll take a look at sorting out the issues that Andrew has raised.

            Show
            ryanwyllie Ryan Wyllie added a comment - Haven't heard back from Ben so I'll take a look at sorting out the issues that Andrew has raised.
            Hide
            ryanwyllie Ryan Wyllie added a comment -

            I've made all of the changes Andrew and Barbara mentioned, except for point 7. I've pushed them all up into my integration master branch (which has the resolved conflicts with integration master) to make it easier to integrate:

            git://github.com/ryanwyllie/moodle.git MDL-372-int-master

            Show
            ryanwyllie Ryan Wyllie added a comment - I've made all of the changes Andrew and Barbara mentioned, except for point 7. I've pushed them all up into my integration master branch (which has the resolved conflicts with integration master) to make it easier to integrate: git://github.com/ryanwyllie/moodle.git MDL-372 -int-master
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Thank you Ben, Charles, and Ryan,

            Your hard work and perseverance means that this issue has now been integrated \o/.

            Integrated to master only. Over to testing.

            Show
            dobedobedoh Andrew Nicols added a comment - Thank you Ben, Charles, and Ryan, Your hard work and perseverance means that this issue has now been integrated \o/. Integrated to master only. Over to testing.
            Hide
            abgreeve Adrian Greeve added a comment -

            Okay, that was quick.
            I immediately get the following error when I navigate to a course.

            Debug info: ERROR: column d.pinned does not exist
            LINE 1: ....usermodified, d.groupid, d.timestart, d.timeend, d.pinned, ...
            ^
            SELECT p.id,p.subject,p.modified,p.discussion,p.userid, d.name, d.timemodified, d.usermodified, d.groupid, d.timestart, d.timeend, d.pinned, u.firstnamephonetic,u.lastnamephonetic,u.middlename,u.alternatename,u.firstname,u.lastname,
            u.email, u.picture, u.imagealt 
            FROM mdl_forum_discussions d
            JOIN mdl_forum_posts p ON p.discussion = d.id
            JOIN mdl_user u ON p.userid = u.id
             
            WHERE d.forum = $1 AND p.parent = 0
             
            ORDER BY d.timemodified DESC, d.id DESC LIMIT 5 OFFSET 0
            [array (
            0 => '1',
            )]
            Error code: dmlreadexception
            Stack trace:
            line 451 of /lib/dml/moodle_database.php: dml_read_exception thrown
            line 244 of /lib/dml/pgsql_native_moodle_database.php: call to moodle_database->query_end()
            line 764 of /lib/dml/pgsql_native_moodle_database.php: call to pgsql_native_moodle_database->query_end()
            line 2608 of /mod/forum/lib.php: call to pgsql_native_moodle_database->get_records_sql()
            line 99 of /blocks/news_items/block_news_items.php: call to forum_get_discussions()
            line 288 of /blocks/moodleblock.class.php: call to block_news_items->get_content()
            line 230 of /blocks/moodleblock.class.php: call to block_base->formatted_contents()
            line 973 of /lib/blocklib.php: call to block_base->get_content_for_output()
            line 1025 of /lib/blocklib.php: call to block_manager->create_block_contents()
            line 474 of /lib/outputrenderers.php: call to block_manager->ensure_content_created()
            line 39 of /theme/bootstrapbase/renderers/core_renderer.php: call to core_renderer->standard_head_html()
            line 52 of /theme/clean/layout/columns3.php: call to theme_bootstrapbase_core_renderer->standard_head_html()
            line 1016 of /lib/outputrenderers.php: call to include()
            line 946 of /lib/outputrenderers.php: call to core_renderer->render_page_layout()
            line 244 of /course/view.php: call to core_renderer->header()
            

            Show
            abgreeve Adrian Greeve added a comment - Okay, that was quick. I immediately get the following error when I navigate to a course. Debug info: ERROR: column d.pinned does not exist LINE 1: ....usermodified, d.groupid, d.timestart, d.timeend, d.pinned, ... ^ SELECT p.id,p.subject,p.modified,p.discussion,p.userid, d.name, d.timemodified, d.usermodified, d.groupid, d.timestart, d.timeend, d.pinned, u.firstnamephonetic,u.lastnamephonetic,u.middlename,u.alternatename,u.firstname,u.lastname, u.email, u.picture, u.imagealt FROM mdl_forum_discussions d JOIN mdl_forum_posts p ON p.discussion = d.id JOIN mdl_user u ON p.userid = u.id   WHERE d.forum = $1 AND p.parent = 0   ORDER BY d.timemodified DESC, d.id DESC LIMIT 5 OFFSET 0 [array ( 0 => '1', )] Error code: dmlreadexception Stack trace: line 451 of /lib/dml/moodle_database.php: dml_read_exception thrown line 244 of /lib/dml/pgsql_native_moodle_database.php: call to moodle_database->query_end() line 764 of /lib/dml/pgsql_native_moodle_database.php: call to pgsql_native_moodle_database->query_end() line 2608 of /mod/forum/lib.php: call to pgsql_native_moodle_database->get_records_sql() line 99 of /blocks/news_items/block_news_items.php: call to forum_get_discussions() line 288 of /blocks/moodleblock.class.php: call to block_news_items->get_content() line 230 of /blocks/moodleblock.class.php: call to block_base->formatted_contents() line 973 of /lib/blocklib.php: call to block_base->get_content_for_output() line 1025 of /lib/blocklib.php: call to block_manager->create_block_contents() line 474 of /lib/outputrenderers.php: call to block_manager->ensure_content_created() line 39 of /theme/bootstrapbase/renderers/core_renderer.php: call to core_renderer->standard_head_html() line 52 of /theme/clean/layout/columns3.php: call to theme_bootstrapbase_core_renderer->standard_head_html() line 1016 of /lib/outputrenderers.php: call to include() line 946 of /lib/outputrenderers.php: call to core_renderer->render_page_layout() line 244 of /course/view.php: call to core_renderer->header()
            Hide
            abgreeve Adrian Greeve added a comment -

            Sorry, about the noise, It seems that there was a problem with my upgrade. Everything seems okay now.

            Show
            abgreeve Adrian Greeve added a comment - Sorry, about the noise, It seems that there was a problem with my upgrade. Everything seems okay now.
            Hide
            abgreeve Adrian Greeve added a comment -

            Tested on master.
            No problems found with the areas tested. Seems to be working okay.
            Test passed.

            Show
            abgreeve Adrian Greeve added a comment - Tested on master. No problems found with the areas tested. Seems to be working okay. Test passed.
            Hide
            marina Marina Glancy added a comment -

            please don't forget to add label "ui_change" together with "docs_required", see https://docs.moodle.org/dev/Tracker_issue_labels
            TIA

            Show
            marina Marina Glancy added a comment - please don't forget to add label "ui_change" together with "docs_required", see https://docs.moodle.org/dev/Tracker_issue_labels TIA
            Hide
            poltawski Dan Poltawski added a comment -

            Thanks for your contributions! This change is now available from the main moodle.git repository and will shortly be available on download.moodle.org.

            The best thing about a boolean is even if you are wrong, you are only off by a bit.

            Show
            poltawski Dan Poltawski added a comment - Thanks for your contributions! This change is now available from the main moodle.git repository and will shortly be available on download.moodle.org. The best thing about a boolean is even if you are wrong, you are only off by a bit.

              People

              • Votes:
                63 Vote for this issue
                Watchers:
                28 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  9/May/16