Moodle
  1. Moodle
  2. MDL-30649

New glossary entries never show up in the recent activity list

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2
    • Fix Version/s: 2.3
    • Component/s: Blocks
    • Labels:
    • Environment:
      LAMP
    • Database:
      MySQL
    • Testing Instructions:
      Hide
      1. Create a recent activities block for a course
      2. Create a glossary activity in the same course
      3. Create a glossary entry
      4. Note the glossary entry is listed in the block
      5. Click the "Full report of recent activity..." link in the block

      Result: Glossary entry reported in recent activity block

      Show
      Create a recent activities block for a course Create a glossary activity in the same course Create a glossary entry Note the glossary entry is listed in the block Click the "Full report of recent activity..." link in the block Result: Glossary entry reported in recent activity block
    • Affected Branches:
      MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      wip-MDL-30649-master
    • Rank:
      33465

      Description

      No matter how you filter the recent activity list, no glossary modifications/additions/etc show up

      Replication steps:

      1. Create a recent activities block for a course
      2. Create a glossary activity in the same course
      3. Create a glossary entry
      4. Note the glossary entry is listed in the block
      5. Click the "Full report of recent activity..." link in the block

      Result: complete lack of glossary activity

        Activity

        Hide
        Jason Fowler added a comment -

        Turns out the functions required to provide this information were missing.

        I have added them now, just wondering if this should be back ported from 2.3Dev to 2.2 and 2.1?

        Should cherry pick cleanly, in theory.

        Show
        Jason Fowler added a comment - Turns out the functions required to provide this information were missing. I have added them now, just wondering if this should be back ported from 2.3Dev to 2.2 and 2.1? Should cherry pick cleanly, in theory.
        Hide
        Jason Fowler added a comment -

        Need to add support for the glossary:view capability for 2.3dev ... will do that as a separate commit so it can be ignored for earlier versions of moodle

        Show
        Jason Fowler added a comment - Need to add support for the glossary:view capability for 2.3dev ... will do that as a separate commit so it can be ignored for earlier versions of moodle
        Hide
        Jason Fowler added a comment -

        That's been added as a different commit, ready for peer review

        Show
        Jason Fowler added a comment - That's been added as a different commit, ready for peer review
        Hide
        Dan Poltawski added a comment -

        Hi Jason,

        Since this is new code it'd be good if we used the new API's, so I would encourage you to look at workshop as a more modern example of how to do the html output.

        But specifically on the HTML output:

        • 'class=forum-recent' is not right
        • Please use moodle_url for the url
        • Use html_writer for the html
        • Avoid using a HTML table if possible

        cheers,
        dan

        Show
        Dan Poltawski added a comment - Hi Jason, Since this is new code it'd be good if we used the new API's, so I would encourage you to look at workshop as a more modern example of how to do the html output. But specifically on the HTML output: 'class=forum-recent' is not right Please use moodle_url for the url Use html_writer for the html Avoid using a HTML table if possible cheers, dan
        Hide
        Jason Fowler added a comment -

        updated the first commit to take into account the changes you recommend Dan, thanks for the help with git and html_writer

        Show
        Jason Fowler added a comment - updated the first commit to take into account the changes you recommend Dan, thanks for the help with git and html_writer
        Hide
        Jason Fowler added a comment -

        Note to integrators: The first commit needs to be cherry picked to 2.0-2.2 where as the second commit stays on 2.3 only.

        Show
        Jason Fowler added a comment - Note to integrators: The first commit needs to be cherry picked to 2.0-2.2 where as the second commit stays on 2.3 only.
        Hide
        Dan Poltawski added a comment -
        1. Why is GLOSSARY_RECENT_ACTIVITY_LIMIT being introduced?
        2. Why is glossary retrieved: $glossary = $DB->get_record('glossary', array('id' => $cm->instance)); ?

        As its not a simple cherry-pick of a single commit (i.e. there is a chance the integrator could ge the cherry-pick the wrong way round or make some mistake) it'd be helpful to prepare seperate branches.

        Show
        Dan Poltawski added a comment - Why is GLOSSARY_RECENT_ACTIVITY_LIMIT being introduced? Why is glossary retrieved: $glossary = $DB->get_record('glossary', array('id' => $cm->instance)); ? As its not a simple cherry-pick of a single commit (i.e. there is a chance the integrator could ge the cherry-pick the wrong way round or make some mistake) it'd be helpful to prepare seperate branches.
        Hide
        Jason Fowler added a comment -

        Okay, I'll remove those lines, I think they were left in from when I copied another function as a starting point, and I didn't realise they weren't needed.

        I will create the other branches once it passes peer review ...

        Show
        Jason Fowler added a comment - Okay, I'll remove those lines, I think they were left in from when I copied another function as a starting point, and I didn't realise they weren't needed. I will create the other branches once it passes peer review ...
        Hide
        Jason Fowler added a comment -

        Changes made and pushed, can you please do one last check and let me know if there is anything else that needs fixing before I create the other branches?

        Show
        Jason Fowler added a comment - Changes made and pushed, can you please do one last check and let me know if there is anything else that needs fixing before I create the other branches?
        Hide
        Dan Poltawski added a comment -

        Heh - sorry. $CFG global decleration is not needed in either function I don't think. After that please submit for integration!

        Show
        Dan Poltawski added a comment - Heh - sorry. $CFG global decleration is not needed in either function I don't think. After that please submit for integration!
        Hide
        Sam Hemelryk added a comment -

        Hi Jason,

        I'm sending this back this week sorry so that the following can be addressed, or at least discussed.

        1. This is down as an improvement and really looks like that is what it is. Policy is not to backport new features or improvements unless there is a good reason to do so. Presently I think this should go to master only, is there a good reason to backport it? (because of this I've only looked at the master branch and the notes reflect the idea of integrating only on master)
        2. Could you please fix the phpdocs for the new functions.
        3. [glossary_get_recent_mod_activity] get_context_instance has been deprecated, all new code should be using the accesslib API (context_module::instance)
        4. [glossary_get_recent_mod_activity] unused vars $joinsql and $clausesql.
        5. [glossary_get_recent_mod_activity] username fields when fetching entries should be coming from user_picture::fields
        6. [glossary_get_recent_mod_activity] That SQL statement would be a whole lot clearer is if were separated from the if, in fact it may be an idea to use a recordset there anyway. Food for throught.
        7. [glossary_get_recent_mod_activity] $usersgroups should be instantiated as null.
        8. [glossary_get_recent_mod_activity] Copy+paste error $attempt doesn't exist
        9. [glossary_get_recent_mod_activity] There appears to be a bug with the group code, when populated the $usersgroups array will contain all of the groups for the user that the first entry not belonging to the current user is a member of. I assume that code is meant to be checking that both the current user and the author of the entry are in the same group, in which case $usergroups is likely the wrong array of groups after the entry not belonging to the current user.
        10. [glossary_get_recent_mod_activity] Also noting $modinfo->groups is deprecated, you should be using $modinfo->get_groups there.
        11. [glossary_get_recent_mod_activity] Alignment when building the $tmpactivity variable is all over the place. It should all be spaced out the same, or it should not be spaced out at all.
        12. [glossary_print_recent_mod_activity] concept and definition likely need to be formatted, or passed through s/p before printing, perhaps this should happen in the previous function when building the content not sure. Either way at the moment it doesn't appear to be cleaned which is a security concern.

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Hi Jason, I'm sending this back this week sorry so that the following can be addressed, or at least discussed. This is down as an improvement and really looks like that is what it is. Policy is not to backport new features or improvements unless there is a good reason to do so. Presently I think this should go to master only, is there a good reason to backport it? (because of this I've only looked at the master branch and the notes reflect the idea of integrating only on master) Could you please fix the phpdocs for the new functions. [glossary_get_recent_mod_activity] get_context_instance has been deprecated, all new code should be using the accesslib API (context_module::instance) [glossary_get_recent_mod_activity] unused vars $joinsql and $clausesql. [glossary_get_recent_mod_activity] username fields when fetching entries should be coming from user_picture::fields [glossary_get_recent_mod_activity] That SQL statement would be a whole lot clearer is if were separated from the if, in fact it may be an idea to use a recordset there anyway. Food for throught. [glossary_get_recent_mod_activity] $usersgroups should be instantiated as null. [glossary_get_recent_mod_activity] Copy+paste error $attempt doesn't exist [glossary_get_recent_mod_activity] There appears to be a bug with the group code, when populated the $usersgroups array will contain all of the groups for the user that the first entry not belonging to the current user is a member of. I assume that code is meant to be checking that both the current user and the author of the entry are in the same group, in which case $usergroups is likely the wrong array of groups after the entry not belonging to the current user. [glossary_get_recent_mod_activity] Also noting $modinfo->groups is deprecated, you should be using $modinfo->get_groups there. [glossary_get_recent_mod_activity] Alignment when building the $tmpactivity variable is all over the place. It should all be spaced out the same, or it should not be spaced out at all. [glossary_print_recent_mod_activity] concept and definition likely need to be formatted, or passed through s/p before printing, perhaps this should happen in the previous function when building the content not sure. Either way at the moment it doesn't appear to be cleaned which is a security concern. Cheers Sam
        Hide
        Jason Fowler added a comment -

        1. not really, I just thought that because it was a change with relatively small impact on other code it could be backported.
        2. Sure.
        3-12. Thanks, I will get on to all these ASAP

        Show
        Jason Fowler added a comment - 1. not really, I just thought that because it was a change with relatively small impact on other code it could be backported. 2. Sure. 3-12. Thanks, I will get on to all these ASAP
        Hide
        Jason Fowler added a comment -

        All those changes are now through Sam, pushing for integration

        Show
        Jason Fowler added a comment - All those changes are now through Sam, pushing for integration
        Hide
        Sam Hemelryk added a comment -

        Thanks Jason, this has been integrated now.

        Just noting I did make an additional commit to fix up whitespace [efd412d].

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Thanks Jason, this has been integrated now. Just noting I did make an additional commit to fix up whitespace [efd412d] . Cheers Sam
        Hide
        Frédéric Massart added a comment -

        As I found that the floating element was breaking the HTML, I also discovered that the definition was appended to the entry content.

        I have written a patch that, I hope, fixes what needs to be fixed. It would probably need to be reviewed because I had to change a few things.

        https://github.com/FMCorz/moodle/commit/3ac4f9dabfa2b74d37c3a30276cc3b916f5a492e

        FYI, I removed the definition for a link to the page of the entry. I also found that the SQL request was returning the wrong id (which was the user one) that's why I added 'entryid'.

        Show
        Frédéric Massart added a comment - As I found that the floating element was breaking the HTML, I also discovered that the definition was appended to the entry content. I have written a patch that, I hope, fixes what needs to be fixed. It would probably need to be reviewed because I had to change a few things. https://github.com/FMCorz/moodle/commit/3ac4f9dabfa2b74d37c3a30276cc3b916f5a492e FYI, I removed the definition for a link to the page of the entry. I also found that the SQL request was returning the wrong id (which was the user one) that's why I added 'entryid'.
        Hide
        Jason Fowler added a comment -

        Hey Fred, could you please attach a screen shot of this "floating element" I was sure I had fixed that...

        Show
        Jason Fowler added a comment - Hey Fred, could you please attach a screen shot of this "floating element" I was sure I had fixed that...
        Hide
        Frédéric Massart added a comment -

        Just uploaded it.

        Show
        Frédéric Massart added a comment - Just uploaded it.
        Hide
        Jason Fowler added a comment -

        Ah, I see, different to the floating issue I had fixed, I hadn't tried anything with longer text for the definition. I've had a look over your patch, and will see what I can do to implement it when I get in tomorrow.

        Show
        Jason Fowler added a comment - Ah, I see, different to the floating issue I had fixed, I hadn't tried anything with longer text for the definition. I've had a look over your patch, and will see what I can do to implement it when I get in tomorrow.
        Hide
        Frédéric Massart added a comment -

        Cool, hope it'll help .
        See you tomorrow!

        Show
        Frédéric Massart added a comment - Cool, hope it'll help . See you tomorrow!
        Hide
        Jason Fowler added a comment -

        Okay, I've cherry picked over Fred's patch - excellent work Fred, very happy with the way that works - and have tested it and pushed it up now for re-integration

        Show
        Jason Fowler added a comment - Okay, I've cherry picked over Fred's patch - excellent work Fred, very happy with the way that works - and have tested it and pushed it up now for re-integration
        Hide
        Aparup Banerjee added a comment -

        looks like you're having fun Fred

        ps: code area was missed out in the commit message. have a read through http://docs.moodle.org/dev/Commit_cheat_sheet#Provide_clear_commit_messages

        Show
        Aparup Banerjee added a comment - looks like you're having fun Fred ps: code area was missed out in the commit message. have a read through http://docs.moodle.org/dev/Commit_cheat_sheet#Provide_clear_commit_messages
        Hide
        Aparup Banerjee added a comment -

        cool, looks good.

        I've picked Fred's commit into integration master (test ran fine for me)

        Show
        Aparup Banerjee added a comment - cool, looks good. I've picked Fred's commit into integration master (test ran fine for me)
        Hide
        Adrian Greeve added a comment -

        I tested this in master and noticed that my entries were turning turning up in the recent activity block, but not in the full report. This patch fixes that problem. I didn't encounter any other problems.
        Thanks Jason.

        Show
        Adrian Greeve added a comment - I tested this in master and noticed that my entries were turning turning up in the recent activity block, but not in the full report. This patch fixes that problem. I didn't encounter any other problems. Thanks Jason.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        This is now part of Moodle and a few millions people around the globe will be using it soon. Isn't that awesome?

        Many, many thanks and don't forget http://youtu.be/4N7dPaP5Z8U

        Closing, ciao

        Show
        Eloy Lafuente (stronk7) added a comment - This is now part of Moodle and a few millions people around the globe will be using it soon. Isn't that awesome? Many, many thanks and don't forget http://youtu.be/4N7dPaP5Z8U Closing, ciao

          People

          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: