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

New glossary entries never show up in the recent activity list

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: 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

      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

        Gliffy Diagrams

          Attachments

            Activity

            Hide
            phalacee 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
            phalacee 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
            phalacee 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
            phalacee 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
            phalacee Jason Fowler added a comment -

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

            Show
            phalacee Jason Fowler added a comment - That's been added as a different commit, ready for peer review
            Hide
            poltawski 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
            poltawski 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
            phalacee 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
            phalacee 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
            phalacee 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
            phalacee 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
            poltawski 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
            poltawski 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
            phalacee 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
            phalacee 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
            phalacee 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
            phalacee 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
            poltawski 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
            poltawski 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
            samhemelryk 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
            samhemelryk 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
            phalacee 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
            phalacee 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
            phalacee Jason Fowler added a comment -

            All those changes are now through Sam, pushing for integration

            Show
            phalacee Jason Fowler added a comment - All those changes are now through Sam, pushing for integration
            Hide
            samhemelryk 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
            samhemelryk 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
            fred 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
            fred 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
            phalacee 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
            phalacee 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
            fred Frédéric Massart added a comment -

            Just uploaded it.

            Show
            fred Frédéric Massart added a comment - Just uploaded it.
            Hide
            phalacee 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
            phalacee 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
            fred Frédéric Massart added a comment -

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

            Show
            fred Frédéric Massart added a comment - Cool, hope it'll help . See you tomorrow!
            Hide
            phalacee 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
            phalacee 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
            nebgor 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
            nebgor 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
            nebgor Aparup Banerjee added a comment -

            cool, looks good.

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

            Show
            nebgor Aparup Banerjee added a comment - cool, looks good. I've picked Fred's commit into integration master (test ran fine for me)
            Hide
            abgreeve 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
            abgreeve 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
            stronk7 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
            stronk7 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:
                  Fix Release Date:
                  25/Jun/12