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
    • Rank:
      6187

      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.)

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

        Issue Links

          Activity

          Hide
          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
          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
          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
          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
          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
          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
          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
          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
          Martin Dougiamas added a comment -

          I like option C too. +1

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

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

          Show
          Aparup Banerjee added a comment - i've created PULL-158 for the 2.0.x fix. looking into 1.9.x now.
          Hide
          Aparup Banerjee added a comment -
          Show
          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 Cai added a comment -

          Peer view:

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

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

          Thanks DS!

          Show
          Aparup Banerjee added a comment - Thanks DS!
          Hide
          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
          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
          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
          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
          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
          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
          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
          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
          Aparup Banerjee added a comment -
          Show
          Aparup Banerjee added a comment - updated fix @ https://github.com/nebgor/moodle/compare/mistress...MDL-22970_m20 please review. (anyone)
          Hide
          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
          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
          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
          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
          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
          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
          Aparup Banerjee added a comment -

          nasty hack aside hoping it looks better.

          Show
          Aparup Banerjee added a comment - nasty hack aside hoping it looks better.
          Hide
          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
          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
          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
          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
          Aparup Banerjee added a comment - work in progress @ https://github.com/nebgor/moodle/compare/mistress...wip-MDL-22970_m20
          Hide
          Aparup Banerjee added a comment -
          Show
          Aparup Banerjee added a comment - ok, peer review can be done @ https://github.com/nebgor/moodle/compare/mistress...MDL-22970_m20
          Hide
          Aparup Banerjee added a comment -

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

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

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

          Show
          Aparup Banerjee added a comment - i'll hold off on porting fixes to 1.9.x until this actually goes past integration.
          Hide
          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
          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
          Helen Foster added a comment -

          Thanks everyone, good work in fixing this issue!

          Show
          Helen Foster added a comment - Thanks everyone, good work in fixing this issue!
          Hide
          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
          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
          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
          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
          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
          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: