search

ovh / php-ovh-sms

PHP for ovh sms API
Other
48 stars 49 forks source link

Possibility to use a user for send a message and bugfix for get sender details #10

Closed Metrakit closed 8 years ago

yadutaf commented 8 years ago

Looks good to me. @golgeek can you confirm?

Metrakit commented 8 years ago

Bonjour, j'ai rajouté un commit qui permet de préciser, en plus du compte, un utilisateur pour envoyer un SMS. Ceci permet de passer par cette URL : https://api.ovh.com/console/#/sms/%7BserviceName%7D/users/%7Blogin%7D/jobs#POST lorsque l'on spécifie un utilisateur avec la méthode SmsApi@setUser.

Plusieurs méthodes ont été rajoutées dans la classe SmsApi :

Cette feature ne change en rien le fonctionnement habituel pour envoyer un message sans préciser un utilisateur.

J'espère que vous allez apprécier ma PR.

Cdlt.

yadutaf commented 8 years ago

Merci pour ta PR !

Je ne suis partagé au sujet du $this->user. Est ce que l'on a vraiment besoin de le rajouter ? Si je suis bien ton patch, la seule fonction qui l'utilise est send mais elle a déjà toutes les informations dans le contexte. Les autres fonctions prennent toutes le $user en paramètre et lèvent une exception s'il n'est pas correctement spécifié. J'ai peur que celà introduise de la confusion.

Une possibilité alternative serait d'utiliser le $this->user comme valeur par défaut dans ces fonctions. Mais je suis partagé. @VincentCasse Tu aurais un avis point de vue dev PHP ?

Metrakit commented 8 years ago

hi @yadutaf , You're welcome.

Oui, je vois ce que tu veux dire mais de quelle façon pourrait-on récupérer l'user dans Message sans passer par un attribut de la classe SmsApi ? En effet, $this->user est pour seulement utiliser dans le setter puis dans le getter afin de pouvoir le récupérer dans la classe Message ici : https://github.com/Metrakit/php-ovh-sms/blob/master/src/Message.php#L172 . Serait-il préférable de rajouter une méthode addUser() dans la classe Message un peu comme les receivers présents ici : https://github.com/Metrakit/php-ovh-sms/blob/master/src/Message.php#L115 ?

Cdlt.

Metrakit commented 8 years ago

Je viens de push un petit fix en rapport avec le getter

golgeek commented 8 years ago

Bonjour,

Merci pour ta PR, c'est une bonne idée d'ajouter les users SMS. Par contre, j'ai peur qu'il soit trompeur d'avoir la gestion des users uniquement sur l'envoi. Tu ne penses pas qu'il serait intéressant d'avoir aussi ce filtrage sur les fonctions getIncomingMessages, getOutgoingMessages et getPlannedMessages ?

Metrakit commented 8 years ago

@golgeek Oui je pense effectivement qu'il faut développer cette feature pour les autres méthodes. Je partage cette PR parce que c'est une feature qu'y m'ai demandé là ou je bosse afin de restreindre l'envoi d'SMS pour un user afin de ne pas tout manger les crédits du compte. J'ai donc décider d'ajouter cette feature à l'API OVH SMS et de la partager. C'est une feature assez bloquante pour le process d'envoi d'SMS pour l'entreprise ou je travail c'est pourquoi j'ai décider de publier rapidement ma PR sans bloquer trop longtemps le projet et sans utiliser un fork. Je peu m'occuper d'améliorer la feature pour getIncomingMessages, getOutgoingMessages et getPlannedMessages en effet. Dois-je reprendre le même procédé que j'ai utilisé pour "send" (cad utiliser getUser() et build l'URL du CURL en fonction de l'user ou non) ?

Metrakit commented 8 years ago

Une méthode du genre : 

    /**
     * Build the URI to use
     * @return string
     */
    private function buildUri()
    {
        $uri = '/sms/' . $this->account . '/';
        if (is_null($this->getUser())) {
            return $uri;
        }
        return $uri . $this->getUser() . '/';
    }

peut-être envisageable dans la classe SmsApi ?

Par exemple à cette ligne : https://github.com/ovh/php-ovh-sms/blob/master/src/SmsApi.php#L363 utiliser : 

  $messages = $this->conn->get($this->getUri() . "outgoing", (object) $parameters);

Si c'est ok je push le commit avec cette feature.

golgeek commented 8 years ago

Ça me semble être une bonne idée, oui. Et ta méthode permettrait de déplacer le check du sender defined dans la méthode getUri() plutôt dans chaque méthode get.

Metrakit commented 8 years ago

Oui ;)

Metrakit commented 8 years ago

@golgeek voilà, je viens de pousser la feature ;)