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

Glossary import displays too many items in recent activity

    Details

    • Difficulty:
      Moderate
    • Affected Branches:
      MOODLE_19_STABLE, MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_19_STABLE, MOODLE_20_STABLE

      Description

      (Note: This probably applies to 1.9 also but I am filing it on 2 because that's where I am testing)

      To reproduce, do the following, and BE AWARE THAT THESE STEPS MAY CRASH YOUR BROWSER.

      1) Create new disposable course
      2) Add a glossary
      3) Import the attached 1,000-entry glossary (note - although the attached example is fake, this is not an unrealistic glossary size - we have several courses using larger)
      4) Return to course page

      Assuming you don't have glossary auto-link on, the 'Recent activity' block will appear with all 1,000 glossary entries shown. This is poor behaviour. I would expect either:

      A) Recent activity shows 'Admin user / Imported 1,000 entries' instead of showing each one individually.
      or
      B) If there are more than, say, 5 new entries then the glossary falls back to just showing '968 new glossary entries'
      or
      C) If there are more than, say, 5 new entries then the glossary displays the first 5 and then '(... and more ...)' or something

      If you have glossary auto-link on, step 4 will crash your browser, presumably because it tries to actually apply the filter (which performs poorly with 1,000 entries, I will file another bug about that) a thousand times and maybe adds some javascript for each one or whatever, I don't know. However, I suspect that if a limit were placed on the number displayed, this would fix the problem anyway.

      (note: by 'crash your browser' I mean, peg its CPU at max; I left it for a minute - Firefox, IE7 - but it didn't finish.)

        Gliffy Diagrams

        1. glossary1000.xml
          371 kB
          Sam Marshall
        1. course.png
          75 kB

          Issue Links

            Activity

            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Assigning to Aparup, raising to critical and sending to stable backlog. This is good candidate for next performance-focussed HQ sprint. Discuss with the Master the best solution of the above but we need, indeed, to limit the number of entries displayed.

            Note this should be fixed both in 1.9.x and 2.0.x

            Thanks for the report, Sam! Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Assigning to Aparup, raising to critical and sending to stable backlog. This is good candidate for next performance-focussed HQ sprint. Discuss with the Master the best solution of the above but we need, indeed, to limit the number of entries displayed. Note this should be fixed both in 1.9.x and 2.0.x Thanks for the report, Sam! Ciao
            Hide
            nebgor Aparup Banerjee added a comment -

            hm, trying to import but got a 'Field "lower(concept)" does not exist in table "glossary_entries"'
            i'll strip that field and try again.

            Show
            nebgor Aparup Banerjee added a comment - hm, trying to import but got a 'Field "lower(concept)" does not exist in table "glossary_entries"' i'll strip that field and try again.
            Hide
            nebgor Aparup Banerjee added a comment -

            sorry that xml is fine, my install was in need of an update. (i wish deleting comments could come with a comment)

            Show
            nebgor Aparup Banerjee added a comment - sorry that xml is fine, my install was in need of an update. (i wish deleting comments could come with a comment)
            Hide
            nebgor Aparup Banerjee added a comment -

            yes a limit seems to fix it. i like option 'C', then we can point to the same link as 'Full report of recent activity...' perhaps.

            Show
            nebgor Aparup Banerjee added a comment - yes a limit seems to fix it. i like option 'C', then we can point to the same link as 'Full report of recent activity...' perhaps.
            Hide
            dougiamas Martin Dougiamas added a comment -

            I like option C too. +1

            Show
            dougiamas Martin Dougiamas added a comment - I like option C too. +1
            Hide
            nebgor Aparup Banerjee added a comment -

            i've created PULL-158 for the 2.0.x fix. looking into 1.9.x now.

            Show
            nebgor Aparup Banerjee added a comment - i've created PULL-158 for the 2.0.x fix. looking into 1.9.x now.
            Hide
            nebgor Aparup Banerjee added a comment -
            Show
            nebgor Aparup Banerjee added a comment - i've created PULL-159 for the 1.9.x fix. ( https://github.com/nebgor/moodle/commit/0745aea69862b17096340850c0e6448883edda2d )
            Hide
            dongsheng Dongsheng Cai added a comment -

            Peer view:

            The option C worked much better than before, the code looks good too +1

            Show
            dongsheng Dongsheng Cai added a comment - Peer view: The option C worked much better than before, the code looks good too +1
            Hide
            nebgor Aparup Banerjee added a comment -

            Thanks DS!

            Show
            nebgor Aparup Banerjee added a comment - Thanks DS!
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Hi Apu, this is being reopened so that more work can be put into glossary_print_recent_activity.
            Please see the comments on PULL-158.

            Cheers
            Sam

            Show
            samhemelryk Sam Hemelryk added a comment - Hi Apu, this is being reopened so that more work can be put into glossary_print_recent_activity. Please see the comments on PULL-158. Cheers Sam
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Hi,

            I'm rejecting both PULL-158 and PULL-159 because I think we can improve things a bit before sending this upstream:

            1) limit the get_records_sql() call to fetch as much as $truncatelimit + 1 records.
            2) within the loop, print up to $truncatelimit records and,
            3) if there are more than $truncatelimit, then and only then, perform the count and print the 'andmorenewentries' string (result of count - $truncatelimit).

            That way we'll be only fetching from DB the required number of records (plus 1) and will execute the count only when necessary.

            Finally, note that you can opt to keep the count out and simply show "and more entries". That would save the count to happen. Please consider both alternatives (ask MD). My +1 goes to perform the count and show the number of "extra" entries.

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Hi, I'm rejecting both PULL-158 and PULL-159 because I think we can improve things a bit before sending this upstream: 1) limit the get_records_sql() call to fetch as much as $truncatelimit + 1 records. 2) within the loop, print up to $truncatelimit records and, 3) if there are more than $truncatelimit, then and only then, perform the count and print the 'andmorenewentries' string (result of count - $truncatelimit). That way we'll be only fetching from DB the required number of records (plus 1) and will execute the count only when necessary. Finally, note that you can opt to keep the count out and simply show "and more entries". That would save the count to happen. Please consider both alternatives (ask MD). My +1 goes to perform the count and show the number of "extra" entries. Ciao
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Also, I think it's ok to fix this for 19_STABLE, as far as it's not only one (general) performance improvement, but one fix for a real breakage happening when too many entries are imported. Please consider / discuss it.

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Also, I think it's ok to fix this for 19_STABLE, as far as it's not only one (general) performance improvement, but one fix for a real breakage happening when too many entries are imported. Please consider / discuss it. Ciao
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            (sending this back to the STABLE backlog. Note we should do that with all the issues belonging to one sprint... or they will be, for sure, be lost).

            http://docs.moodle.org/en/Development:Testing already shows that, Integration should do the same IMO.

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - (sending this back to the STABLE backlog. Note we should do that with all the issues belonging to one sprint... or they will be, for sure, be lost). http://docs.moodle.org/en/Development:Testing already shows that, Integration should do the same IMO. Ciao
            Hide
            nebgor Aparup Banerjee added a comment -
            Show
            nebgor Aparup Banerjee added a comment - updated fix @ https://github.com/nebgor/moodle/compare/mistress...MDL-22970_m20 please review. (anyone)
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Hi Apu,

            I've read through the code on this issue and come up with the following notes:
            mod/glossary/lib.php

            • Line 345: user_picture::fields() should be used to get the user fields
            • Line 350:
              1. Don't put this in one line. Retrieve $entries and then check it for what ever you want, likely !empty().
              2. $truncatelimit+1 is a nasty, nasty hack
              3. Never write LIMIT directly into the SQL, it isn't supported by all DB's (sqlsrv + oracle) use arguments 3 & 4 for get_records_sql
            • Line 366: You should preload contexts in the SQL to save on DB queries within the foreach: context_instance_preload_sql + context_instance_preload

            To be truthful I think the approach needs to be re-thought-out. I personally think check for mod/glossary:approve isn't needed at all? Surely that should be mod/glossary:view or equivalent?
            I'm not too sure what other modules are doing but I think it should be checked and a standard or even semi standard approach taken.
            What if you in the first query select all the glossaries in the course along with their caps, then check the view cap, and finally select recent activity where the user has the view cap in the glossary the entry belongs to?
            You'd still only have two queries, however you would be able to properly limit the second query that fetches the entry. It would be accurate and I still don't think it would be to painful.
            Ohhh come to think of it you could even join to the entries table and then only select glossaries that have recent entries... even better!

            Either way I think more work is required yet sorry.

            Cheers
            Sam

            Show
            samhemelryk Sam Hemelryk added a comment - Hi Apu, I've read through the code on this issue and come up with the following notes: mod/glossary/lib.php Line 345: user_picture::fields() should be used to get the user fields Line 350: Don't put this in one line. Retrieve $entries and then check it for what ever you want, likely !empty(). $truncatelimit+1 is a nasty, nasty hack Never write LIMIT directly into the SQL, it isn't supported by all DB's (sqlsrv + oracle) use arguments 3 & 4 for get_records_sql Line 366: You should preload contexts in the SQL to save on DB queries within the foreach: context_instance_preload_sql + context_instance_preload To be truthful I think the approach needs to be re-thought-out. I personally think check for mod/glossary:approve isn't needed at all? Surely that should be mod/glossary:view or equivalent? I'm not too sure what other modules are doing but I think it should be checked and a standard or even semi standard approach taken. What if you in the first query select all the glossaries in the course along with their caps, then check the view cap, and finally select recent activity where the user has the view cap in the glossary the entry belongs to? You'd still only have two queries, however you would be able to properly limit the second query that fetches the entry. It would be accurate and I still don't think it would be to painful. Ohhh come to think of it you could even join to the entries table and then only select glossaries that have recent entries... even better! Either way I think more work is required yet sorry. Cheers Sam
            Hide
            nebgor Aparup Banerjee added a comment -

            Thanks Sam,
            i've cleaned up the first 2 points (lines 345/350). I'm looking up the one about contexts.

            also i think mod/glossary:approve is about showing approved glossary entries, as in the approval process of entering data into the glossary..

            Show
            nebgor Aparup Banerjee added a comment - Thanks Sam, i've cleaned up the first 2 points (lines 345/350). I'm looking up the one about contexts. also i think mod/glossary:approve is about showing approved glossary entries, as in the approval process of entering data into the glossary..
            Hide
            nebgor Aparup Banerjee added a comment -

            ok , i've put in the preloading caches @ https://github.com/nebgor/moodle/compare/mistress...MDL-22970_m20

            please have a look Sam.

            Show
            nebgor Aparup Banerjee added a comment - ok , i've put in the preloading caches @ https://github.com/nebgor/moodle/compare/mistress...MDL-22970_m20 please have a look Sam.
            Hide
            nebgor Aparup Banerjee added a comment -

            nasty hack aside hoping it looks better.

            Show
            nebgor Aparup Banerjee added a comment - nasty hack aside hoping it looks better.
            Hide
            nebgor Aparup Banerjee added a comment -

            changes pushed up to https://github.com/nebgor/moodle/compare/mistress...MDL-22970_m20
            aside from trying to saving a max of 6 has_capability checks by squeezing it into the main query.. how does the patch look? --> Sam

            Show
            nebgor Aparup Banerjee added a comment - changes pushed up to https://github.com/nebgor/moodle/compare/mistress...MDL-22970_m20 aside from trying to saving a max of 6 has_capability checks by squeezing it into the main query.. how does the patch look? --> Sam
            Hide
            nebgor Aparup Banerjee added a comment -

            note: As Sam mentioned, its still not nice at all when a student almost gets to see the 5 limit of new glossary entries when alas they all haven't been approved yet and none get displayed (to a role with no approval caps). We should show the student the 5 limit that have been approved.

            so we need to do the cap checks before fetching the data perhaps.

            Show
            nebgor Aparup Banerjee added a comment - note: As Sam mentioned, its still not nice at all when a student almost gets to see the 5 limit of new glossary entries when alas they all haven't been approved yet and none get displayed (to a role with no approval caps). We should show the student the 5 limit that have been approved. so we need to do the cap checks before fetching the data perhaps.
            Show
            nebgor Aparup Banerjee added a comment - work in progress @ https://github.com/nebgor/moodle/compare/mistress...wip-MDL-22970_m20
            Hide
            nebgor Aparup Banerjee added a comment -
            Show
            nebgor Aparup Banerjee added a comment - ok, peer review can be done @ https://github.com/nebgor/moodle/compare/mistress...MDL-22970_m20
            Hide
            nebgor Aparup Banerjee added a comment -

            i've just written up a test for this in PULL-249

            Show
            nebgor Aparup Banerjee added a comment - i've just written up a test for this in PULL-249
            Hide
            nebgor Aparup Banerjee added a comment -

            i'll hold off on porting fixes to 1.9.x until this actually goes past integration.

            Show
            nebgor Aparup Banerjee added a comment - i'll hold off on porting fixes to 1.9.x until this actually goes past integration.
            Hide
            nebgor Aparup Banerjee added a comment -

            create PULL-306 for the 1.9 patch. This fix should be all done now for 1.9.x and 2.0.x

            Show
            nebgor Aparup Banerjee added a comment - create PULL-306 for the 1.9 patch. This fix should be all done now for 1.9.x and 2.0.x
            Hide
            tsala Helen Foster added a comment -

            Thanks everyone, good work in fixing this issue!

            Show
            tsala Helen Foster added a comment - Thanks everyone, good work in fixing this issue!
            Hide
            smithrn Ryan Smith added a comment -

            I updated to Moodle 1.9.11 and it looks like this bug fix was added to that version. Now, when you add a glossary entry and return to the main course page, you cannot add any new blocks even though editing is turned on. I reverted mod/glossary/lib.php to the previous version and it fixes the problem.

            Show
            smithrn Ryan Smith added a comment - I updated to Moodle 1.9.11 and it looks like this bug fix was added to that version. Now, when you add a glossary entry and return to the main course page, you cannot add any new blocks even though editing is turned on. I reverted mod/glossary/lib.php to the previous version and it fixes the problem.
            Hide
            nebgor Aparup Banerjee added a comment -

            Re-opening issue, it seems there is a problem with 1.9+ fix based on report from Ryan Smith.

            Show
            nebgor Aparup Banerjee added a comment - Re-opening issue, it seems there is a problem with 1.9+ fix based on report from Ryan Smith.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Hi Aparup,

            I'm closing this again and assigning you to MDL-26757, where the problem has been tracked with more detail.

            It seems that one moodle_url() use arrived as part of the backport, escaped to my integration and wasn't detected on testing either.

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Hi Aparup, I'm closing this again and assigning you to MDL-26757 , where the problem has been tracked with more detail. It seems that one moodle_url() use arrived as part of the backport, escaped to my integration and wasn't detected on testing either. Ciao

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  21/Feb/11