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

Add Signatures Support #1616

Open
wants to merge 9 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@tahaalibra
Contributor

tahaalibra commented Aug 9, 2016

this will add signature support

todo

  • use clob in database
  • update composer to use signature
  • use service updating signature in frontend
  • remove files and comments not needed
  • work on UI
  • remove travis errors

Note: This is in development, and as of current commit many things will be changed

@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Aug 9, 2016

@tahaalibra, thanks for your PR! By analyzing the annotation information on this pull request, we identified @ChristophWurst, @DeepDiver1975 and @Gomez to be potential reviewers

mention-bot commented Aug 9, 2016

@tahaalibra, thanks for your PR! By analyzing the annotation information on this pull request, we identified @ChristophWurst, @DeepDiver1975 and @Gomez to be potential reviewers

@tahaalibra

This comment has been minimized.

Show comment
Hide comment
@tahaalibra

tahaalibra Aug 9, 2016

Contributor

what works:

  • backend complete
  • loading and saving the signatures through the account setting works

i would recommend not review this PR on current commit
@ChristophWurst @Gomez

Contributor

tahaalibra commented Aug 9, 2016

what works:

  • backend complete
  • loading and saving the signatures through the account setting works

i would recommend not review this PR on current commit
@ChristophWurst @Gomez

@tahaalibra

This comment has been minimized.

Show comment
Hide comment
@tahaalibra

tahaalibra Aug 9, 2016

Contributor

i have a question, when replying do we include signature?

Contributor

tahaalibra commented Aug 9, 2016

i have a question, when replying do we include signature?

@ChristophWurst

This comment has been minimized.

Show comment
Hide comment
@ChristophWurst

ChristophWurst Aug 10, 2016

Contributor

yes, signatures should be used for replies too

Contributor

ChristophWurst commented Aug 10, 2016

yes, signatures should be used for replies too

tahaalibra added some commits Aug 11, 2016

Show outdated Hide outdated appinfo/database.xml
<field>
<name>signature</name>
<type>clob</type>
<notnull>true</notnull>

This comment has been minimized.

@ChristophWurst

ChristophWurst Aug 15, 2016

Contributor

why notnull?

@ChristophWurst

ChristophWurst Aug 15, 2016

Contributor

why notnull?

Show outdated Hide outdated js/service/signatureservice.js
* @param signature
* @returns {undefined}
*/
function saveAccountSignature(accountId, signature) {

This comment has been minimized.

@ChristophWurst

ChristophWurst Aug 15, 2016

Contributor

pass an Account object instead, that's what we do in other services too

@ChristophWurst

ChristophWurst Aug 15, 2016

Contributor

pass an Account object instead, that's what we do in other services too

Show outdated Hide outdated js/views/composer.js
},
showSignature: function() {
var alias = this.findAliasById(this.$('.mail-account').
find(':selected').val());

This comment has been minimized.

@ChristophWurst

ChristophWurst Aug 15, 2016

Contributor

indent this line

@ChristophWurst

ChristophWurst Aug 15, 2016

Contributor

indent this line

Show outdated Hide outdated js/views/composer.js
var alias = this.findAliasById(this.$('.mail-account').
find(':selected').val());
if (!this.isReply()) {
$('textarea[name="body"]').val('\n\n' + alias.signature);

This comment has been minimized.

@ChristophWurst

ChristophWurst Aug 15, 2016

Contributor

use this.$('textarea[name="body"]')

@ChristophWurst

ChristophWurst Aug 15, 2016

Contributor

use this.$('textarea[name="body"]')

Show outdated Hide outdated tests/db/mailaccounttest.php
@@ -52,7 +53,8 @@ public function testToAPI() {
'smtpHost' => 'smtp.marvel.com',
'smtpPort' => 458,
'smtpUser' => 'spiderman',
'smtpSslMode' => 'ssl'
'smtpSslMode' => 'ssl',
'signature' => 'Thanks You'

This comment has been minimized.

@ChristophWurst

ChristophWurst Aug 15, 2016

Contributor

by default it should be null

@ChristophWurst

ChristophWurst Aug 15, 2016

Contributor

by default it should be null

@ChristophWurst

This comment has been minimized.

Show comment
Hide comment
@ChristophWurst

ChristophWurst Aug 15, 2016

Contributor

tahaalibra added enhancement 3 - to review in progress labels 6 days ago

Please only set the 3 - to review if you're confident the code is ready to merge (all code written, tests fixed, no unchecked TODOs)

Contributor

ChristophWurst commented Aug 15, 2016

tahaalibra added enhancement 3 - to review in progress labels 6 days ago

Please only set the 3 - to review if you're confident the code is ready to merge (all code written, tests fixed, no unchecked TODOs)

tahaalibra added some commits Aug 16, 2016

Show outdated Hide outdated js/service/signatureservice.js
var defer = $.Deferred();
var url = OC.generateUrl('/apps/mail/accounts/{id}/signature', {
id: accountId
id: account.accountId

This comment has been minimized.

@ChristophWurst

ChristophWurst Aug 16, 2016

Contributor

did you test this? I assume account.accountId will be undefined as it should be a Backbone model and you have to use its get method instead to access the id.

@ChristophWurst

ChristophWurst Aug 16, 2016

Contributor

did you test this? I assume account.accountId will be undefined as it should be a Backbone model and you have to use its get method instead to access the id.

@mmattel

This comment has been minimized.

Show comment
Hide comment
@mmattel

mmattel Aug 16, 2016

1.)
Are signatures differentiated between initial eMail and reply? (like you can do in Outlook)
Means:
Initial: eg long text version with much content
Reply: eg short text version with minimum content

Case needed: Business eMail need long and short signatures depending on the eMail type

2.)
When you have multiple accounts, can you have for each account a different signature set?

mmattel commented Aug 16, 2016

1.)
Are signatures differentiated between initial eMail and reply? (like you can do in Outlook)
Means:
Initial: eg long text version with much content
Reply: eg short text version with minimum content

Case needed: Business eMail need long and short signatures depending on the eMail type

2.)
When you have multiple accounts, can you have for each account a different signature set?

@ChristophWurst

This comment has been minimized.

Show comment
Hide comment
@ChristophWurst

ChristophWurst Aug 16, 2016

Contributor

Are signatures differentiated between initial eMail and reply?

No, they aren't. I see that it would make sense, but let's keep the first version simple. Please open an enhancement ticket as soon as this feature was merged, so someone can work on that later. Thanks :-)

When you have multiple accounts, can you have for each account a different signature set?

Yes, you can. Also for different aliases of an account.

Contributor

ChristophWurst commented Aug 16, 2016

Are signatures differentiated between initial eMail and reply?

No, they aren't. I see that it would make sense, but let's keep the first version simple. Please open an enhancement ticket as soon as this feature was merged, so someone can work on that later. Thanks :-)

When you have multiple accounts, can you have for each account a different signature set?

Yes, you can. Also for different aliases of an account.

Show outdated Hide outdated lib/service/accountservice.php
$account = $this->mapper->find($currentUserId, $accountId);
$account->setSignature($signature);
return $this->mapper->update($account);
} catch(Exception $e) {

This comment has been minimized.

@ChristophWurst

ChristophWurst Aug 16, 2016

Contributor

Hm. The service should catch implementation specific exceptions (e.g. DB exception) and throw an own exception instead. Maybe something like ServiceException? Example: https://github.com/nextcloud/news/blob/master/lib/Service/ServiceException.php

@ChristophWurst

ChristophWurst Aug 16, 2016

Contributor

Hm. The service should catch implementation specific exceptions (e.g. DB exception) and throw an own exception instead. Maybe something like ServiceException? Example: https://github.com/nextcloud/news/blob/master/lib/Service/ServiceException.php

Show outdated Hide outdated tests/service/accoutservicetest.php
$accountId = 123;
$signature = "Thank You";
$this->mapper->expects($this->once())

This comment has been minimized.

@ChristophWurst

ChristophWurst Aug 16, 2016

Contributor

is there a unit test for the mapper to ensure this method call works, too?

@ChristophWurst

ChristophWurst Aug 16, 2016

Contributor

is there a unit test for the mapper to ensure this method call works, too?

@Gomez

This comment has been minimized.

Show comment
Hide comment
@Gomez

Gomez Sep 22, 2016

Contributor

@tahaalibra Should we move this over? Someone still on it?

Contributor

Gomez commented Sep 22, 2016

@tahaalibra Should we move this over? Someone still on it?

@mariomurrent-softwaresolutions

This comment has been minimized.

Show comment
Hide comment
@mariomurrent-softwaresolutions

mariomurrent-softwaresolutions May 12, 2017

@tahaalibra Is your work finished? I would need this functionality asap. Maybe you can contact me.

@tahaalibra Is your work finished? I would need this functionality asap. Maybe you can contact me.

@Gomez

This comment has been minimized.

Show comment
Hide comment
@Gomez

Gomez May 12, 2017

Contributor

@mariomurrent-softwaresolutions This is not merged. Any help is appreciated, this PR can be used as a base.
Regarding signatures support please continue here:
nextcloud/mail#190

The Mail app repository was moved:
https://github.com/nextcloud/mail

Contributor

Gomez commented May 12, 2017

@mariomurrent-softwaresolutions This is not merged. Any help is appreciated, this PR can be used as a base.
Regarding signatures support please continue here:
nextcloud/mail#190

The Mail app repository was moved:
https://github.com/nextcloud/mail

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