Moodle
  1. Moodle
  2. MDL-25341

blog_sync_external_entries() doesn't sync but delete all and fetch all

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.0
    • Fix Version/s: 2.0.2
    • Component/s: Blog
    • Labels:
    • Database:
      Any
    • Difficulty:
      Moderate
    • Affected Branches:
      MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE

      Description

      It seems that blog_sync_external_entries(), in charge of getting one rss source doesn't sync selectively but, instead, deletes all the entries and load all them again.

      This have 2 important side effects:

      • Order can become garbled, as far as post date fallbacks to time(), so we can be pushing external entries up all the time, causing nightmares to own generated rss, visualization...
      • Old external entries are lost. Once the rss source stop publishing one item, it disappears from moodle too. That shouldn't happen.

      So we need to read the rss and then, comparing by dates, by hash, or by content (or fallbacking between them), decide which entries must be inserted, but never deleting old ones.

      Ciao

        Gliffy Diagrams

          Activity

          Hide
          David Jones added a comment -

          For what it's worth, the BIM module (not in contrib - http://github.com/djplaner/BIM) for Moodle 1.9 uses the permalink as the unique id. Which seems to be what is used in Moodle 2.0 stored in the uniquehash field of the post table.

          This is probably not 100% as I think some blogs allow this to be changed. Sorry, not that helpful.

          Show
          David Jones added a comment - For what it's worth, the BIM module (not in contrib - http://github.com/djplaner/BIM ) for Moodle 1.9 uses the permalink as the unique id. Which seems to be what is used in Moodle 2.0 stored in the uniquehash field of the post table. This is probably not 100% as I think some blogs allow this to be changed. Sorry, not that helpful.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Assigning this initially to Andrew (the component lacks "boss" right now). Moving to stable backlog and marking as triaged.

          Andrew, plz, don't forget to comment this in next HQ meeting, because we have now like 60 bugs for the blog component (http://tracker.moodle.org/browse/MDL/component/10095) needing love. So we must have one component leader asap.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Assigning this initially to Andrew (the component lacks "boss" right now). Moving to stable backlog and marking as triaged. Andrew, plz, don't forget to comment this in next HQ meeting, because we have now like 60 bugs for the blog component ( http://tracker.moodle.org/browse/MDL/component/10095 ) needing love. So we must have one component leader asap. TIA and ciao
          Hide
          Andrew Davis added a comment - - edited

          Ready for peer review by Sam git://github.com/andyjdavis/moodle.git
          branch is blog_MDL-25341_entry_sync
          diff https://github.com/andyjdavis/moodle/compare/master...blog_MDL-25341_entry_sync

          Show
          Andrew Davis added a comment - - edited Ready for peer review by Sam git://github.com/andyjdavis/moodle.git branch is blog_ MDL-25341 _entry_sync diff https://github.com/andyjdavis/moodle/compare/master...blog_MDL-25341_entry_sync
          Hide
          Sam Hemelryk added a comment -

          Hi Andrew,

          I've just had a look at it everything looks good however there were two things spotted:
          line 208: $newentry->created = $entrydate; should probably be left as it is, seeing as we aren't deleting everything now it would be nice to keep the original date it was posted?
          line 232: update record returns true not the id.

          I was also thinking about what happens if someone deletes an entry from their external blog, I think you raised this in the meeting this morning? I take it we arn't concerned and if they want to I suppose they can delete it from Moodle as well, although we won't do it automatically.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Andrew, I've just had a look at it everything looks good however there were two things spotted: line 208: $newentry->created = $entrydate; should probably be left as it is, seeing as we aren't deleting everything now it would be nice to keep the original date it was posted? line 232: update record returns true not the id. I was also thinking about what happens if someone deletes an entry from their external blog, I think you raised this in the meeting this morning? I take it we arn't concerned and if they want to I suppose they can delete it from Moodle as well, although we won't do it automatically. Cheers Sam
          Hide
          Andrew Davis added a comment -

          Will figure out the issue with the post creation time in the morning. It would certainly be nice to hold onto the original creation time.

          re post deletion theres an issue there because, for example, someone may set their blog rss feed to only show the last 10 blog posts. An item not appearing in the rss feed hasnt necessarily been deleted. It may just no longer appear in the feed. I thought we could do some sort of date range checking ie if an item is missing and we have items older than it its been deleted, however this seemed more complex than was necessary. Instead Moodle's copies of external blog rss feeds will operate as a Google cache style copy of long lost content.

          Show
          Andrew Davis added a comment - Will figure out the issue with the post creation time in the morning. It would certainly be nice to hold onto the original creation time. re post deletion theres an issue there because, for example, someone may set their blog rss feed to only show the last 10 blog posts. An item not appearing in the rss feed hasnt necessarily been deleted. It may just no longer appear in the feed. I thought we could do some sort of date range checking ie if an item is missing and we have items older than it its been deleted, however this seemed more complex than was necessary. Instead Moodle's copies of external blog rss feeds will operate as a Google cache style copy of long lost content.
          Hide
          Andrew Davis added a comment -

          All done. The git hub links above are still good.

          Show
          Andrew Davis added a comment - All done. The git hub links above are still good.
          Hide
          Andrew Davis added a comment -

          have added code to remove deleted blog posts. Peer review please

          Show
          Andrew Davis added a comment - have added code to remove deleted blog posts. Peer review please
          Hide
          Sam Hemelryk added a comment -

          Hi Andrew,

          Just been checking out the patch and testing it.
          In testing everything worked as I expected (once I worked out I had to delete the simplepie cache).
          In regards to code only a couple of minor white space issues round if statements and an array
          lib.php: 206, 226, 231, 245

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Andrew, Just been checking out the patch and testing it. In testing everything worked as I expected (once I worked out I had to delete the simplepie cache). In regards to code only a couple of minor white space issues round if statements and an array lib.php: 206, 226, 231, 245 Cheers Sam
          Hide
          Andrew Davis added a comment -

          thats done

          Show
          Andrew Davis added a comment - thats done
          Hide
          Sam Hemelryk added a comment -

          Hi Andrew,

          I didn't worry about testing it this time (did that last time).
          Thanks for fixing up the white-space, unfortunately I spotted two more lines 260, and 263.
          It has my +1 once those have been fixed.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Andrew, I didn't worry about testing it this time (did that last time). Thanks for fixing up the white-space, unfortunately I spotted two more lines 260, and 263. It has my +1 once those have been fixed. Cheers Sam
          Hide
          Andrew Davis added a comment -

          Thanks Sam. Fixed those.

          PULL-54

          Show
          Andrew Davis added a comment - Thanks Sam. Fixed those. PULL-54
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Closing as fixed. Will land to upstream in hours.

          Thanks & ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Closing as fixed. Will land to upstream in hours. Thanks & ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: