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

      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

        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: