Skip to content

RemoveClassifiedEventActivity: check if calendar still exists - #13503

Merged
nickvergessen merged 1 commit into
masterfrom
fix/13499
Jan 11, 2019
Merged

RemoveClassifiedEventActivity: check if calendar still exists#13503
nickvergessen merged 1 commit into
masterfrom
fix/13499

Conversation

@rullzer

@rullzerrullzer commented Jan 11, 2019

Copy link
Copy Markdown
Member

Thnx for spotting @go2sh

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

ChristophWurst commented Jan 11, 2019

Copy link
Copy Markdown
Member

What about #13499 (comment)?

@rullzer

rullzer commented Jan 11, 2019

Copy link
Copy Markdown
MemberAuthor

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

ChristophWurst
ChristophWurst previously approved these changes Jan 11, 2019
View reviewed changes

@ChristophWurstChristophWurst left a comment

Copy link
Copy Markdown
Member

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

go2sh commented Jan 11, 2019

Copy link
Copy Markdown
Contributor

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

@rullzer

rullzer commented Jan 11, 2019

Copy link
Copy Markdown
MemberAuthor

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

@rullzer

rullzer commented Jan 11, 2019

Copy link
Copy Markdown
MemberAuthor

(i.e. this patch breaks them)

@rullzer

rullzer commented Jan 11, 2019

Copy link
Copy Markdown
MemberAuthor

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

@nickvergessennickvergessen left a comment

Copy link
Copy Markdown
Member

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

ChristophWurst commented Jan 11, 2019

Copy link
Copy Markdown
Member

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 reviewJanuary 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>
@rullzerrullzer changed the title Use the correct column names in RemoveClassifiedEventActivityRemoveClassifiedEventActivity: check if calendar still existsJan 11, 2019
nickvergessen
nickvergessen approved these changes Jan 11, 2019
View reviewed changes
Comment threadapps/dav/lib/Migration/RemoveClassifiedEventActivity.php

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

@nickvergessennickvergessenJan 11, 2019

Copy link
Copy Markdown
Member

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.

@MorrisJobkeMorrisJobkeJan 11, 2019

Copy link
Copy Markdown
Member

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

@ChristophWurstChristophWurst left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Makes sense!

@ChristophWurstChristophWurst mentioned this pull request Jan 11, 2019
Closed
@nickvergessen
nickvergessen merged commit e7b9746 into masterJan 11, 2019
@nickvergessen
nickvergessen deleted the fix/13499 branch January 11, 2019 10:30
@nickvergessen

nickvergessen commented Jan 11, 2019

Copy link
Copy Markdown
Member

/backport to stable15

@nickvergessen

nickvergessen commented Jan 11, 2019

Copy link
Copy Markdown
Member

/backport to stable14

@nickvergessen

nickvergessen commented Jan 11, 2019

Copy link
Copy Markdown
Member

/backport to stable13

@backportbot-nextcloudbackportbot-nextcloudBot mentioned this pull request Jan 11, 2019
Merged
@backportbot-nextcloud

backportbot-nextcloudBot commented Jan 11, 2019

Copy link
Copy Markdown

backport to stable15 in #13506

@backportbot-nextcloudbackportbot-nextcloudBot mentioned this pull request Jan 11, 2019
Merged
@backportbot-nextcloud

backportbot-nextcloudBot commented Jan 11, 2019

Copy link
Copy Markdown

backport to stable14 in #13507

@backportbot-nextcloudbackportbot-nextcloudBot mentioned this pull request Jan 11, 2019
Merged
@backportbot-nextcloud

backportbot-nextcloudBot commented Jan 11, 2019

Copy link
Copy Markdown

backport to stable13 in #13508

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

Reviewers

@go2shgo2shgo2sh approved these changes
@nickvergessennickvergessennickvergessen approved these changes
@ChristophWurstChristophWurstChristophWurst approved these changes
@MorrisJobkeMorrisJobkeAwaiting requested review from MorrisJobke

Assignees

No one assigned

Labels

3. to reviewWaiting for reviews

Projects

None yet

Milestone

Nextcloud 16

Development

Successfully merging this pull request may close these issues.

5 participants

@rullzer@ChristophWurst@go2sh@nickvergessen@MorrisJobke