-
Notifications
You must be signed in to change notification settings - Fork 28
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
Perguntas Anónimas #62
base: main
Are you sure you want to change the base?
Conversation
Corrigidos problemas de formatação |
} | ||
|
||
if ( | ||
this.message.channel.type === "DM" && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think there should be a ENUM here for types
|
||
if ( | ||
this.message.channel.type === "DM" && | ||
this.message.content.startsWith("!pergunta") && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
☝️
…os ids dos canais.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Enviei algumas pontos que vi que ainda podemos melhorar.
Se quiseres podemos discutir alguns dos pontos no Discord 👍
import ChannelResolver from "../../domain/service/channelResolver"; | ||
|
||
const askedRecently = new Set(); | ||
class DirectMessage { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Por norma temos declarado as classes com o mesmo nome do ficheiro e com o sufixo UseCase. Consegues alterar?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
feito
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
feito
constructor(private message: Message, private client: Client, private channelResolver: ChannelResolver) {} | ||
|
||
async validate() { | ||
if (askedRecently.has(this.message.author.id)) { | ||
this.message.channel.send("Ainda não podes enviar outra pergunta. Tenta mais tarde."); | ||
} else { | ||
const chatService: ChatService = new DiscordChatService(this.client); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
O ChatService recebes no constructor
, não tens de o criar.
As dependências são inicializadas no index.ts
e depois passadas para cada UseCase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
feito
class DirectMessage { | ||
constructor(private message: Message, private client: Client, private channelResolver: ChannelResolver) {} | ||
|
||
async validate() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tendo em conta que a função validate
retorna um bool
e não um void
, sugiro um isValid
, penso fazer mais sentido.
E assim o teu if poderia ser um if (await isValid()) {
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
feito
setTimeout(() => { | ||
askedRecently.delete(this.message.author.id); | ||
}, 60000); | ||
return true; | ||
} | ||
chatService.sendDM(this.message.author.id, "Por favor usa o seguinte formato:\n!pergunta <mensagem>"); | ||
return false; | ||
} | ||
return true; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
O que achas de uma abordagem mais deste género? Penso que fique mais explícito que tem que esperar os 60s para executar a ação seguinte.
setTimeout(() => { | |
askedRecently.delete(this.message.author.id); | |
}, 60000); | |
return true; | |
} | |
chatService.sendDM(this.message.author.id, "Por favor usa o seguinte formato:\n!pergunta <mensagem>"); | |
return false; | |
} | |
return true; | |
} | |
await this.timeout(60000); | |
askedRecently.delete(this.message.author.id); | |
return true; | |
} | |
chatService.sendDM(this.message.author.id, "Por favor usa o seguinte formato:\n!pergunta <mensagem>"); | |
return false; | |
} | |
return true; | |
} | |
function timeout(ms) { | |
return new Promise(resolve => setTimeout(resolve, ms)); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
só ficou esta por fazer porque ainda não percebi bem, tenho que ver com mais tempo, entretanto tento resolver !
this.message.channel.type === "DM" && | ||
this.message.content.startsWith("!pergunta") && | ||
this.message.content.split(" ").length > 1 && | ||
this.message.content.length <= 1500 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Podemos passar o 1500
para uma constante (mesmo que dentro desta classe)?
É preferível ficar num nome percetível do que significa este valor do que ficar um valor arbitrário no código.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
feito
domain/service/chatService.ts
Outdated
@@ -1,3 +1,5 @@ | |||
export default interface ChatService { | |||
sendMessageToChannel(message: string, channelId: string): void; | |||
deleteMessageFromChannel(messageId: string, channelId: string): void; | |||
sendDM(message: string, userId: string): void; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Preferível ser mais explícito e evitar acrónimos.
sendDM(message: string, userId: string): void; | |
sendDirectMessageToUser(message: string, userId: string): void; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
feito
index.ts
Outdated
const validationCheck = await directMessage.validate(); | ||
if (validationCheck) { | ||
directMessage.messageApprove(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Como referi num comment acima, se mudarmos o naming da função podemos inclusive melhorar a legibilidade deste check e até usar early returns
para a validação. (se quisermos até podemos dar logo embed do await no if.
const validationCheck = await directMessage.validate(); | |
if (validationCheck) { | |
directMessage.messageApprove(); | |
} | |
const isValid = await directMessage.isValid(); | |
if (isValid) { | |
return; | |
} | |
directMessage.messageApprove(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
feito
index.ts
Outdated
.setFooter({ text: `Aprovado por ${userName}` }); | ||
|
||
switch (interaction.customId) { | ||
case "bt1": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Este Ids deveriam ser algo mais específico caso queiramos vir a adicionar novos botões mais tarde.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
feito
index.ts
Outdated
default: { | ||
console.log("default"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Presumo que possamos retirar isto daqui?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
não retirei porque senão não passa no lint, dá para desativar esse check ?
try { | ||
const user = await this.client.users.fetch(userId); | ||
await user.send(message); | ||
return true; | ||
} catch (e) { | ||
console.error(e); | ||
return false; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tendo em conta que não estamos a fazer nada com o resultado desta função (bool
), estamos a retornar isso por alguma razão?
Presumo que possamos inclusive retirar o try/catch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
e feito
readDirectMessage.ts - Alterada a class DirectMessage para ReadDirectMessageUseCase - Alterado o nome do método sendDM para sendDirectMessageToUser - Removida a inicialização do ChatService e agora é passado como parametro de constructor - Removidos elses desnecessários - Na criação dos botoes, o ID do user que envia a mensagem é passado com a propriedade customId (apr+originalUserId) ou (rej+originalUserId) index.ts - no switch statement é feito um slice para comparar os 3 primeiros caracteres do customId para verificar se é apr(aprovar) ou rej(rejeitar) discordChatService - alterada a função sendDirectMessageToUser para void e retirado try catch
Criação de novos ficheiros de serviço:
Mudanças no ficheiro index.ts:
Criação do ficheiro application/usecases/readDirectMessage.ts:
Mudanças no ficheiro domain/service/chatService.ts:
Criação do ficheiro domain/service/dmService.ts:
Infelizmente, não foi possível implementar a funcionalidade de enviar a pergunta anónima para uma thread nova num fórum após aprovação, pois não há forma de testar essa funcionalidade de forma adequada. Isso é devido à falta de acesso a uma plataforma de fórum para configurar e testar essa funcionalidade. Portanto, essa funcionalidade não pode ser incluída nesta versão.