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

Public sharing #1

Merged
merged 46 commits into from
Oct 14, 2016
Merged

Public sharing #1

merged 46 commits into from
Oct 14, 2016

Conversation

georgehrke
Copy link
Member

No description provided.

@georgehrke georgehrke added the 3. to review Waiting for reviews label Sep 16, 2016
@georgehrke
Copy link
Member Author

@tcitworld

@raghunayyar
Copy link
Member

Can you add the steps to check?
This is one massive Pull Request. :)

@georgehrke georgehrke mentioned this pull request Sep 18, 2016
1 task
@coveralls
Copy link

coveralls commented Sep 19, 2016

Coverage Status

Coverage decreased (-0.5%) to 25.262% when pulling e7e5e33 on public-sharing into 5d7b19f on master.

@tcitworld
Copy link
Member

tcitworld commented Sep 19, 2016

@raghunayyar

  • Check that it doesn't break anything without the server change : CalDAV calendar public sharing server#1197 (i.e work on NC < 11).
  • Test that the features comes correctly with the server change
  • Test publish link is accessible and contains all events
  • Test email is correctly sent with link

Todos :

  • More tests can be added
  • OpenGraph informations on a link

@coveralls
Copy link

coveralls commented Sep 19, 2016

Coverage Status

Coverage decreased (-0.5%) to 25.262% when pulling 2c6b339 on public-sharing into 5d7b19f on master.

@coveralls
Copy link

coveralls commented Sep 19, 2016

Coverage Status

Coverage decreased (-0.5%) to 25.191% when pulling a06fb63 on public-sharing into 30b3eae on master.

@georgehrke
Copy link
Member Author

pr in server: nextcloud/server#1197

@georgehrke
Copy link
Member Author

@tcitworld Can you take care of rebasing? :)

@tcitworld
Copy link
Member

How did you know ? I was doing it right now !

@georgehrke
Copy link
Member Author

hehe 🙈

@coveralls
Copy link

coveralls commented Sep 22, 2016

Coverage Status

Coverage decreased (-0.5%) to 25.221% when pulling 56dbb6a on public-sharing into 76c0b64 on master.

@coveralls
Copy link

coveralls commented Sep 26, 2016

Coverage Status

Coverage decreased (-0.6%) to 27.887% when pulling 674af79 on public-sharing into 6499ac9 on master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.6%) to 27.887% when pulling 674af79 on public-sharing into 6499ac9 on master.

@coveralls
Copy link

coveralls commented Sep 27, 2016

Coverage Status

Coverage decreased (-0.6%) to 27.898% when pulling 856d761 on public-sharing into 8c471dd on master.

@georgehrke
Copy link
Member Author

nextcloud/server#1197 was merged, so let's get this in by end of the week :)

@tcitworld
Copy link
Member

It's the end of the week. ;-) Not sure if 856d761 works properly.

@georgehrke
Copy link
Member Author

nextcloud chromium today at 2 53 26 pm

I'm not too happy, that these are in the settings. It's not really settings and has a bad discoverability I think. But we can take care of this in a follow-up PR. not a blocker

@georgehrke
Copy link
Member Author

georgehrke commented Oct 1, 2016

  • please only add the public graph thing to main.php when it's a public page

@georgehrke
Copy link
Member Author

calendar - nextcloud chromium today at 3 24 47 pm

Send button jumps into next line when list is scrollable


$message = $this->mailer->createMessage();
$message->setSubject($subject);
$message->setFrom([$sendFrom => 'ownCloud Notifier']);
Copy link
Member Author

Choose a reason for hiding this comment

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

🙈

}
$this->mailer->send($message);

return new JSONResponse([], Http::STATUS_OK);
Copy link
Member Author

Choose a reason for hiding this comment

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

the JSONResponse call should be moved to sendEmailPublicLink

@georgehrke
Copy link
Member Author

Right now we are only sending an html-body. Please also provide a plain text message.
see http://swiftmailer.org/docs/messages.html#setting-the-body-content

We should not force users to use html emails.

@georgehrke
Copy link
Member Author

Looking at the source code of OCA\Mail\Message, I suppose that you first have to use setPlainBody and afterwords set the html content with setHtmlBody

https://github.com/nextcloud/server/blob/8c7d7d7746e76b77ad86cee3aae5dbd4d1bcd896/lib/private/Mail/Message.php#L211

@livier livier mentioned this pull request Dec 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants