Skip to content

Conversation

@MailyLehoux
Copy link
Contributor

resolve #377

@MailyLehoux MailyLehoux self-assigned this Feb 15, 2023
@MailyLehoux MailyLehoux changed the title WIP 🧐 add notification table WIP ✨ add noification section Feb 22, 2023
@MailyLehoux MailyLehoux changed the title WIP ✨ add noification section WIP ✨ add notification section Feb 22, 2023
@MailyLehoux MailyLehoux changed the title WIP ✨ add notification section ✨ add notification section Feb 27, 2023
@MailyLehoux MailyLehoux requested a review from bengeois February 27, 2023 10:42
Copy link
Member

@bengeois bengeois left a comment

Choose a reason for hiding this comment

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

  1. Hasura migrations should be refactored to avoid lots of migration (especially migrations that are not used like multiple rename migration).

  2. Instead of having a relationship between a notification and a skill, I think it would be better to have the text of the notification directly in the database. Because in the future, there might be other notifications that don't need the skills

  3. Same thing of having a creator column in the Skill table, in this way we inform the creator only but not other consultants that add this skill during the skill validation period.

What do you think ?

Otherwise, this new feature is really nice and might be used for multiple informations 🦾

### MG_NOTIFICATIONS

> If admin approve a skill added by a user, the user receive a notification
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
> If admin approve a skill added by a user, the user receive a notification
> If admin approves a skill added by a user, the user receives a notification

typo

Comment on lines +126 to +131
notification: {
notificationText:
"L'administrateur adminMail a approuvé votre compétence skillName comme nouvelle compétence Skillz",
new: "Nouveau",
notificationTypeAccepted: "Une de tes compétence a été acceptée",
},
Copy link
Member

Choose a reason for hiding this comment

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

It's weird to have the you "singular" and "plurial" on the same page.

Comment on lines +85 to +88
color?: string | null | undefined;
x?: string | null | undefined;
y?: string | null | undefined;
index?: number | null | undefined;
Copy link
Member

Choose a reason for hiding this comment

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

I'm the opinion of using Partial<CategoryItem> where it is needed instead of change the database object representation in the wrong way (for example, color is never null in the database)

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.

Notification when Admin approuved a skill

3 participants