ecomplus / app-ses

E-Com Plus app for AWS SES as default email server
MIT License
1 stars 2 forks source link

Simplificar admin settings #18

Closed leomp12 closed 4 years ago

leomp12 commented 4 years ago

Precisamos atualizar o admin settings deste app conforme o que foi mencionado no Slack:

Todo app deve ter o máximo de defaults possível, a configuração "blockante" é só que for realmente requerido, evitar arrays e objetos, toggles (booleanos) geralmente são melhores que inputs...

Para cada tipo de e-mail deve haver booleanos para lojista e cliente no formato disable_${email}_customer e disable_${email}_merchant (ex.: disable_welcome_customer e disable_welcome_merchant), é importante que os titles (e talvez descriptions) de cada campo sejam descritivos.

Desta forma todos os e-mails serão habilitados por padrão, o app funcionará enviando todos os e-mails assim que instalado, o lojista poderá desabilitar e-mails alterando o toggle de cada booleano.

leomp12 commented 4 years ago

@talissonf se for trabalhoso refatorar o app, você pode apenas parsear o config, seta um objeto baseConfig padrão (referente ao admin settings atual) com o array trigger preenchido com todas as opções, depois percorre o config do lojista e pra cada booleano true você remove a opção respectiva no baseConfig.

talissonf commented 4 years ago

Eu acho trabalhoso refatorar toda a configuração, ainda mais que o app ja está instalado em várias lojas. O que acontecia antes é que o admin-marketplace não entende valores defaults para tipo boleano daí eu deixava os gatilhos com default true, na view era mostrado como marcado mas quando salva os valores do form ele não vem. Por isso mudei a configuração deixando os boleanos como default false pro lojista ser obrigado a habilitar o que ele precisa usar até isso ser corrigido no admin-marketplace.

se for trabalhoso refatorar o app, você pode apenas parsear o config, seta um objeto baseConfig padrão (referente ao admin settings atual) com o array trigger preenchido com todas as opções, depois percorre o config do lojista e pra cada booleano true você remove a opção respectiva no baseConfig.

Mas se for o caso da configuração está confusa eu faço como ce sugiu.

leomp12 commented 4 years ago

O problema é o lojista ter que preencher o array triggers (com o enum) pra ver o app funcionar. Mesmo com os bugs do admin marketplace, a configuração deste app tem que ser mais simples do que a atual, assim como de outros apps comuns.

Não mexa em apps instalados nas outras lojas (isto tem que ser evitado sempre), você pode manter o suporte pra configuração anterior checando se o triggers existe e é um array no objeto config, se for você não faz o parse.

leomp12 commented 4 years ago

O ponto é ter apenas booleanos para ativar ou desativar os emails, evitando o array de objetos.

Usar disable ou enable é um minor, mas neste caso sugeri disable porque todos os emails serão considerados ativos por padrão, então acho que faz mais sentido desabilitar.

leomp12 commented 4 years ago

Pra não ficar tudo embolado (pricipalmente quando isto https://github.com/ecomplus/app-ses/issues/19 for implementado), talvez o ideal seja um objeto predefinido para cada e-mail e os booleanos dentro dele, ex.:

"welcome": {
  "type": "object",
  "properties": {
    "disable_customer": {
      "type": "boolean"
    },
    "disable_merchant": {
      "type": "boolean"
    }
  } 
}

Ficaria organizado em accordion com um collapse para cada e-mail no admin marketplace.

talissonf commented 4 years ago

https://github.com/ecomplus/app-ses/compare/044dd001f5...89c4ab6c3e

talissonf commented 4 years ago

Inclui nas configurações também a escolha do idioma dos e-mails agora.

leomp12 commented 4 years ago

Boa @talissonf , mas o lang se não for definido será pt_br, né?

talissonf commented 4 years ago

Sim @leomp12