Skip to content
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.

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

Fix calendar emails to be outlook compatible #12946

Closed
wants to merge 7 commits into from

Conversation

hdijkema
Copy link

@hdijkema hdijkema commented Dec 9, 2018

Signed-off-by: hdijkema hans@dykema.nl

@jospoortvliet
Copy link
Member

ref #12885 - nicely done with lots of comments, hartelijk dank @hdijkema ! Now let's see if @georgehrke can review...

@georgehrke
Copy link
Member

supersedes #12921

@georgehrke
Copy link
Member

cc @nickvergessen regarding the changes to Mailer

@georgehrke
Copy link
Member

Can you please cover your changes with Unit tests? Thx! :)

@hdijkema
Copy link
Author

Can you please cover your changes with Unit tests? Thx! :)

Where can I find the current unit tests?

@kesselb
Copy link
Contributor

kesselb commented Dec 10, 2018

Sorry to interrupt. Is it possible that this patch is related to the timezone issue you are describing? sabre-io/vobject#412

@hdijkema
Copy link
Author

Sorry to interrupt. Is it possible that this patch is related to the timezone issue you are describing? sabre-io/vobject#412

This fix in Sabre solves the DTSTART / DTEND TZID problem for Outlook 2016 and 2010. I hope it will be part of nextcloud soon. So we can leave out the timezone fix in IMipPlugin.php.

hdijkema and others added 7 commits January 15, 2019 00:27
Signed-off-by: hdijkema <hans@dykema.nl>
Co-Authored-By: hdijkema <hans@dykema.nl>
Signed-off-by: hdijkema <hans@dykema.nl>
Co-Authored-By: hdijkema <hans@dykema.nl>
Signed-off-by: hdijkema <hans@dykema.nl>
Co-Authored-By: hdijkema <hans@dykema.nl>
Signed-off-by: hdijkema <hans@dykema.nl>
Co-Authored-By: hdijkema <hans@dykema.nl>
Signed-off-by: hdijkema <hans@dykema.nl>
Signed-off-by: hdijkema <hans@dykema.nl>
Copy link
Contributor

@kesselb kesselb left a comment

Choose a reason for hiding this comment

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

Thank you! Looks good so far.

* @return $this
* @since 16.0.0
*/
public function addPart($data, $content_type = null, $charset = null): IMessage {
Copy link
Contributor

Choose a reason for hiding this comment

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

The method here works different than addPart https://github.com/nextcloud/3rdparty/blob/da8e24b48079cec2f9c8be950f7b996bce172190/swiftmailer/swiftmailer/lib/classes/Swift/Message.php#L72 (from swiftmailer). I would prefer another name than because addPart from swiftmailer add a part with the current encoder while these method always uses 8bit encoder. I would add getEncoder and setEncoder and set the encoding from IMipPlugin but lets ask @nickvergessen first.

@georgehrke
Copy link
Member

@hdijkema Can you please comment and/or adapt to @kesselb 's comment? :)

#12946 (comment)

@hdijkema
Copy link
Author

@hdijkema Can you please comment and/or adapt to @kesselb 's comment? :)

#12946 (comment)

No problem at all to change the name of addPart(). Be my guest.

@nickvergessen nickvergessen changed the title Implementation of #12885 Fix calendar emails to be outlook compatible Sep 24, 2019
This was referenced Dec 11, 2019
@hdijkema
Copy link
Author

hdijkema commented Dec 12, 2019

There seems to be no record for the failure:

https://drone.nextcloud.com/nextcloud/server/14710

results in a 404. See below?

@kesselb
Copy link
Contributor

kesselb commented Dec 12, 2019

Builds logs are deleted after some time to save space. Rebase the branch to trigger CI again.

@rullzer rullzer mentioned this pull request Dec 19, 2019
18 tasks
@rullzer rullzer modified the milestones: Nextcloud 18, Nextcloud 19 Dec 19, 2019
This was referenced Apr 4, 2020
This was referenced Apr 15, 2020
@rullzer rullzer mentioned this pull request Apr 23, 2020
11 tasks
# encoder with an 8bit encoder and after we've finished, we reset the encoder
# to the previous one.
$encoder = $this->swiftMessage->getEncoder();
$eightbit_encoder = new \Swift_Mime_ContentEncoder_PlainContentEncoder("8bit");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$eightbit_encoder = new \Swift_Mime_ContentEncoder_PlainContentEncoder("8bit");
$eightbitEncoder = new \Swift_Mime_ContentEncoder_PlainContentEncoder('8bit');

# to the previous one.
$encoder = $this->swiftMessage->getEncoder();
$eightbit_encoder = new \Swift_Mime_ContentEncoder_PlainContentEncoder("8bit");
$this->swiftMessage->setEncoder($eightbit_encoder);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$this->swiftMessage->setEncoder($eightbit_encoder);
$this->swiftMessage->setEncoder($eightbitEncoder);

* @return $this
* @since 16.0.0
*/
public function addPart($data, $content_type = null, $charset = null): IMessage {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public function addPart($data, $content_type = null, $charset = null): IMessage {
public function addPart($data, $contentType = null, $charset = null): IMessage {

$encoder = $this->swiftMessage->getEncoder();
$eightbit_encoder = new \Swift_Mime_ContentEncoder_PlainContentEncoder("8bit");
$this->swiftMessage->setEncoder($eightbit_encoder);
$this->swiftMessage->addPart($data, $content_type, $charset);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$this->swiftMessage->addPart($data, $content_type, $charset);
$this->swiftMessage->addPart($data, $contentType, $charset);

@rullzer rullzer modified the milestones: Nextcloud 19, Nextcloud 20 Apr 30, 2020
@ChristophWurst
Copy link
Member

@hdijkema 🏓 :)

@hdijkema
Copy link
Author

hdijkema commented Jul 2, 2020

@hdijkema 🏓 :)

?

@ChristophWurst
Copy link
Member

There are pending changes from the review and

Rebase the branch to trigger CI again.

Could you look into that? Cheers

@hdijkema
Copy link
Author

hdijkema commented Jul 2, 2020

There are pending changes from the review and

Rebase the branch to trigger CI again.

Could you look into that? Cheers

There are pending changes from the review and

Rebase the branch to trigger CI again.

Could you look into that? Cheers

I'm sorry, I'm lost here. Don't know what to do. I moved back to owncloud after problems with nextcloud and have not been back to nextcloud again the last year. I prepared this patch about 2 years ago, because back then I wanted nextcloud to send proper invites for calendar items. Got it working; next thing I find was this mergerequest stuck on integration checks that I don't understand.

It also was stuck on waiting for a new release of the underlying calendar library (forgot it's name). Maybe you can just review what I've done and put the right changes in the NextCloud tree yourself?

@ChristophWurst
Copy link
Member

Ok, then I'll close it and anyone who would like to continue this work can just restore the branch and do the rest.

merlinwoff added a commit to merlinwoff/server that referenced this pull request Sep 29, 2022
merlinwoff added a commit to merlinwoff/server that referenced this pull request Sep 29, 2022
Signed-off-by: merlinwoff <merlin.woff@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants