Skip to content

Add DAV repair step to fix calendar data#2273

Merged
LukasReschke merged 1 commit intomasterfrom
fix_invalid_calendar
Dec 20, 2016
Merged

Add DAV repair step to fix calendar data#2273
LukasReschke merged 1 commit intomasterfrom
fix_invalid_calendar

Conversation

@rullzer
Copy link
Copy Markdown
Member

@rullzer rullzer commented Nov 23, 2016

Should fix #2264

We add a live repair step to revalidate all calendar entries. Because some might have invalid vcard data in them.

@MorrisJobke @LukasReschke as discussed.

@mention-bot
Copy link
Copy Markdown

mention-bot commented Nov 23, 2016

@rullzer, thanks for your PR! By analyzing the history of the files in this pull request, we identified @DeepDiver1975, @nickvergessen and @LukasReschke to be potential reviewers.

@rullzer rullzer added this to the Nextcloud 11.0 milestone Nov 23, 2016
@rullzer rullzer force-pushed the fix_invalid_calendar branch from 4a43f4b to cf30b85 Compare November 23, 2016 11:31
nickvergessen
nickvergessen reviewed Nov 23, 2016
View reviewed changes
apps/dav/tests/unit/Migration/ValueFixTest.php Outdated
STATUS:CONFIRMED
SEQUENCE:0
SUMMARY:Ein Geburtstag
DTSTART:20151223T173000Z
Copy link
Copy Markdown
Member

@nickvergessen nickvergessen Nov 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

break here and on CREATED as well?

Copy link
Copy Markdown
Member Author

@rullzer rullzer Nov 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean replace with DTSTART;VALUE=:20151223T173000Z ?

Copy link
Copy Markdown
Member

@nickvergessen nickvergessen Nov 23, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, if that is also something that should be fixed from broken iOS event

Copy link
Copy Markdown
Member Author

@rullzer rullzer Nov 24, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well basically the ;VALUE=: is just invalid. (Type hinting can go on almost any field). The regex is simple and just replaces that.

I can add it to DTSTART as well if you prefer

Copy link
Copy Markdown
Member Author

@rullzer rullzer Nov 24, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@rullzer rullzer force-pushed the fix_invalid_calendar branch from cf30b85 to 55f0197 Compare November 24, 2016 08:37
MorrisJobke
MorrisJobke previously requested changes Nov 24, 2016
View reviewed changes
apps/dav/lib/Migration/ValueFixInsert.php Outdated
}

public function run(IOutput $output) {
$this->userManager->callForSeenUsers(function (IUser $user) {
Copy link
Copy Markdown
Member

@MorrisJobke MorrisJobke Nov 24, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a check, that this is only done once? Otherwise on every minor upgrade this is re-inserted.

Copy link
Copy Markdown
Member Author

@rullzer rullzer Nov 24, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@rullzer
Add DAV repair step to fix calendar data
6f5316e 
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
@rullzer rullzer force-pushed the fix_invalid_calendar branch from 55f0197 to 6f5316e Compare November 24, 2016 19:31
@codecov-io
Copy link
Copy Markdown

codecov-io commented Nov 24, 2016

Current coverage is 56.91% (diff: 94.44%)

Merging #2273 into master will decrease coverage by 0.12%

@@             master      #2273   diff @@
==========================================
  Files          1190       1192     +2   
  Lines         71729      72173   +444   
  Methods        7294       7320    +26   
  Messages          0          0          
  Branches       1213       1309    +96   
==========================================
+ Hits          40913      41078   +165   
- Misses        30816      31093   +277   
- Partials          0          2     +2

Powered by Codecov. Last update 10db5bf...6f5316e

@MorrisJobke
Copy link
Copy Markdown
Member

MorrisJobke commented Nov 30, 2016

I inserted following event into the calendar:

BEGIN:VCALENDAR
VERSION:2.0
PRODID:-//Apple Inc.//Mac OS X 10.11.2//EN
CALSCALE:GREGORIAN
BEGIN:VEVENT
TRANSP:OPAQUE
DTEND;VALUE=:20151223T223000Z
LAST-MODIFIED:20151214T091032Z
ORGANIZER;CN="User 1":mailto:user1@example.com
UID:1234567890@example.com
DTSTAMP:20151214T091032Z
STATUS:CONFIRMED
SEQUENCE:0
SUMMARY:Ein Geburtstag 
DTSTART:20151223T173000Z
X-APPLE-TRAVEL-ADVISORY-BEHAVIOR:AUTOMATIC
CREATED:20151214T091032Z
END:VEVENT
END:VCALENDAR

Then did an ./occ upgrade and ran the cron job multiple times php cron.php afterwards the calendar is still throwing the error.

@rullzer Any ideas?

@rullzer
Copy link
Copy Markdown
Member Author

rullzer commented Nov 30, 2016

@MorrisJobke mmm maybe something else is still broken. I'll have a look soon.

@rullzer
Copy link
Copy Markdown
Member Author

rullzer commented Dec 2, 2016

@MorrisJobke I can't reproduce that.

What I did:

  1. Install stable10
  2. Upload file to /remote.php/dav/calendars//personal
  3. Checkout branch
  4. ./occ up
  5. php cron.php (few times)
  6. In calendar go to December 2015
  7. No 💥

@LukasReschke
Copy link
Copy Markdown
Member

LukasReschke commented Dec 11, 2016

@rullzer Can we move that to 12? I'd prefer to consider 11 closed for any non-critical merges.

@rullzer
Copy link
Copy Markdown
Member Author

rullzer commented Dec 12, 2016

Uhm. We'll sure as we mitigated the upgrade bug. But not having this means some people can still have invalid data in the db.

But fine by me

@nickvergessen nickvergessen mentioned this pull request Dec 19, 2016
Closed
@rullzer
Copy link
Copy Markdown
Member Author

rullzer commented Dec 20, 2016

@MorrisJobke lets get this in. Then we can take a look at your issue separatly

@nickvergessen @LukasReschke good to go from you?

MorrisJobke
MorrisJobke approved these changes Dec 20, 2016
View reviewed changes
Copy link
Copy Markdown
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least fixes some cases 👍

@LukasReschke LukasReschke merged commit f0bb62f into master Dec 20, 2016
@LukasReschke LukasReschke deleted the fix_invalid_calendar branch December 20, 2016 22:09
@rullzer rullzer mentioned this pull request Dec 21, 2016
Merged
@rullzer
Copy link
Copy Markdown
Member Author

rullzer commented Dec 21, 2016

Stable11 in #2807

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nickvergessen nickvergessen nickvergessen left review comments

@MorrisJobke MorrisJobke MorrisJobke approved these changes

@LukasReschke LukasReschke LukasReschke approved these changes

Assignees

No one assigned

Labels

3. to review Waiting for reviews bug feature: dav

Projects

None yet

Milestone

Nextcloud 12.0

Development

Successfully merging this pull request may close these issues.

Fix invalid VCALENDAR data in DB!

6 participants

@rullzer @mention-bot @codecov-io @MorrisJobke @LukasReschke @nickvergessen