Skip to content

RemoveClassifiedEventActivity: check if calendar still exists #13503

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Jan 11, 2019
Merged

RemoveClassifiedEventActivity: check if calendar still exists #13503

merged 1 commit into from
Jan 11, 2019

Conversation

@rullzer
Copy link
Member

@rullzer rullzer commented Jan 11, 2019

Thnx for spotting @go2sh

@rullzer rullzer added the 3. to review Waiting for reviews label Jan 11, 2019
@rullzer rullzer added this to the Nextcloud 16 milestone Jan 11, 2019
@ChristophWurst
Copy link
Member

ChristophWurst commented Jan 11, 2019

What about #13499 (comment)?

@rullzer
Copy link
Member Author

rullzer commented Jan 11, 2019

Yeah that should be fixed since we now fetch the right principal

ChristophWurst
ChristophWurst previously approved these changes Jan 11, 2019
View reviewed changes
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Diff makes sense 👍

Not tested yet, but trying to reproduce now

@go2sh
Copy link
Contributor

go2sh commented Jan 11, 2019

I did update our instance to 15.0.1, but I was not able to reproduce it.

@rullzer
Copy link
Member Author

rullzer commented Jan 11, 2019

Mmmm this is weird. On my own setup the column names are correct.

@rullzer
Copy link
Member Author

rullzer commented Jan 11, 2019

(i.e. this patch breaks them)

@rullzer
Copy link
Member Author

rullzer commented Jan 11, 2019

maybe the problem comes from some events that do not have a proper principal uri set?

nickvergessen
nickvergessen requested changes Jan 11, 2019
View reviewed changes
Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

This does not work. The array does not have the table aliases on the entries

@ChristophWurst
Copy link
Member

ChristophWurst commented Jan 11, 2019

This does not work. The array does not have the table aliases on the entries

Right
bildschirmfoto von 2019-01-11 09-27-23

(upgrade to master)

@ChristophWurst ChristophWurst dismissed their stale review January 11, 2019 08:32

does not seem to be the correct fix

@rullzer
Check for no longer existing calendars
12e24ed 
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
@rullzer rullzer changed the title Use the correct column names in RemoveClassifiedEventActivity RemoveClassifiedEventActivity: check if calendar still exists Jan 11, 2019
nickvergessen
nickvergessen approved these changes Jan 11, 2019
View reviewed changes
apps/dav/lib/Migration/RemoveClassifiedEventActivity.php

while ($row = $result->fetch()) {
if ($row['principaluri'] === null) {
continue;
Copy link
Member

@nickvergessen nickvergessen Jan 11, 2019

Choose a reason for hiding this comment

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

Actually we should delete those right away. But I will add a new repair step to check this.

Copy link
Member

@MorrisJobke MorrisJobke Jan 11, 2019

Choose a reason for hiding this comment

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

Why wasn't this added to the SQL statement? 😉

ChristophWurst
ChristophWurst approved these changes Jan 11, 2019
View reviewed changes
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Makes sense!

@ChristophWurst ChristophWurst mentioned this pull request Jan 11, 2019
Closed
@nickvergessen nickvergessen merged commit e7b9746 into master Jan 11, 2019
@nickvergessen nickvergessen deleted the fix/13499 branch January 11, 2019 10:30
@nickvergessen
Copy link
Member

nickvergessen commented Jan 11, 2019

/backport to stable15

@nickvergessen
Copy link
Member

nickvergessen commented Jan 11, 2019

/backport to stable14

@nickvergessen
Copy link
Member

nickvergessen commented Jan 11, 2019

/backport to stable13

@backportbot-nextcloud backportbot-nextcloud bot mentioned this pull request Jan 11, 2019
Merged
@backportbot-nextcloud
Copy link

backportbot-nextcloud bot commented Jan 11, 2019

backport to stable15 in #13506

@backportbot-nextcloud backportbot-nextcloud bot mentioned this pull request Jan 11, 2019
Merged
@backportbot-nextcloud
Copy link

backportbot-nextcloud bot commented Jan 11, 2019

backport to stable14 in #13507

@backportbot-nextcloud backportbot-nextcloud bot mentioned this pull request Jan 11, 2019
Merged
@backportbot-nextcloud
Copy link

backportbot-nextcloud bot commented Jan 11, 2019

backport to stable13 in #13508

@nickvergessen nickvergessen mentioned this pull request Jan 11, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@go2sh go2sh go2sh approved these changes

@nickvergessen nickvergessen nickvergessen approved these changes

@ChristophWurst ChristophWurst ChristophWurst approved these changes

@MorrisJobke MorrisJobke Awaiting requested review from MorrisJobke

Assignees

No one assigned

Labels

3. to review Waiting for reviews

Projects

None yet

Milestone

Nextcloud 16

Development

Successfully merging this pull request may close these issues.

6 participants

@rullzer @ChristophWurst @go2sh @nickvergessen @MorrisJobke