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

Perguntas Anónimas #62

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Perguntas Anónimas #62

wants to merge 13 commits into from

Conversation

zorkpt
Copy link
Contributor

@zorkpt zorkpt commented Jan 29, 2023

Criação de novos ficheiros de serviço:

  • discordChatService.ts, discordDmService.ts e discordEmbedService.ts foram adicionados para separar as responsabilidades de comunicação com o Discord

Mudanças no ficheiro index.ts:

  • Uma nova classe DirectMessage foi adicionada para validação e aprovação de mensagens diretas
  • Um novo evento "interactionCreate" foi adicionado para lidar com interações de botões

Criação do ficheiro application/usecases/readDirectMessage.ts:

  • A lógica de validação e aprovação de mensagens diretas foi adicionada

Mudanças no ficheiro domain/service/chatService.ts:

  • Uma nova interface foi adicionada para representar um serviço de chat (deleteMessage)

Criação do ficheiro domain/service/dmService.ts:

  • Uma nova interface foi adicionada para representar um serviço de mensagens diretas

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.

@zorkpt
Copy link
Contributor Author

zorkpt commented Jan 31, 2023

Corrigidos problemas de formatação

}

if (
this.message.channel.type === "DM" &&
Copy link

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") &&
Copy link

Choose a reason for hiding this comment

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

☝️

index.ts Outdated Show resolved Hide resolved
@zorkpt zorkpt requested a review from ALPHPOST February 6, 2023 17:24
@zorkpt zorkpt requested review from Adjilino and removed request for ALPHPOST February 7, 2023 18:07
Copy link
Member

@IvoPereira IvoPereira left a 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 {
Copy link
Member

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

feito

Copy link
Contributor Author

Choose a reason for hiding this comment

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

feito

Comment on lines 11 to 17
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);
Copy link
Member

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.

Copy link
Contributor Author

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() {
Copy link
Member

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()) {.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

feito

Comment on lines 29 to 38
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;
}
Copy link
Member

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.

Suggested change
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));
}

Copy link
Contributor Author

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
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

feito

@@ -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;
Copy link
Member

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.

Suggested change
sendDM(message: string, userId: string): void;
sendDirectMessageToUser(message: string, userId: string): void;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

feito

index.ts Outdated
Comment on lines 76 to 79
const validationCheck = await directMessage.validate();
if (validationCheck) {
directMessage.messageApprove();
}
Copy link
Member

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.

Suggested change
const validationCheck = await directMessage.validate();
if (validationCheck) {
directMessage.messageApprove();
}
const isValid = await directMessage.isValid();
if (isValid) {
return;
}
directMessage.messageApprove();

Copy link
Contributor Author

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":
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

feito

index.ts Outdated
Comment on lines 119 to 121
default: {
console.log("default");
}
Copy link
Member

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?

Copy link
Contributor Author

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 ?

Comment on lines 35 to 42
try {
const user = await this.client.users.fetch(userId);
await user.send(message);
return true;
} catch (e) {
console.error(e);
return false;
}
Copy link
Member

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.

Copy link
Contributor Author

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants