-
Notifications
You must be signed in to change notification settings - Fork 71
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
📢 Add support for role mentions #4789
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
Edit: Approved by mistake, let me review 😅
@@ -26,7 +26,7 @@ export const toNotificationEvents = | |||
const build = buildEvents(allMemberIds, event) | |||
|
|||
const notifEvent = match(event) | |||
.with({ __typename: 'PostAddedEvent' }, (e) => fromPostAddedEvent(e, build)) | |||
.with({ __typename: 'PostAddedEvent' }, (e) => fromPostAddedEvent(e, build, roles)) | |||
.with({ __typename: 'ThreadCreatedEvent' }, (e) => fromThreadCreatedEvent(e, build)) |
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 we forgot to pass roles
here? If so, can you also add a test that would catch this?
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.
Yeah I forgot about the thread creation mid way. I added some tests and improved the NotifEventFromQNEvent
type to prevent this.
if (!groupId) { | ||
return workers.map((worker) => Number(worker.membershipId)) | ||
} |
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 don't think we need to support mention of only workers. DAO + Council + All leads + WG workers + WG lead is good enough
Hey @ivanturlakov I deployed this version of the backend at: https://api-rul8.onrender.com it's pointed to Mainnet. On this PR preview: https://dao-git-feature-dao-roles-support-joystream.vercel.app/ could you please replace the backend url on the custom network and then:
If this goes I think that's enough QA for this feature. |
Tested on https://dao-git-feature-dao-roles-support-joystream.vercel.app/#/forum/thread/875 My email(Gmail) address was not linked and the app asked me to add it, the confirmation email went to the spam folder. I confirmed the email and my address appeared in Settings > Notifications I created a new thread and tagged Builders I added a post to the thread and tagged DAO It's been 3 hours so far and I haven't received any notifications yet(including spam folder) |
@ivanturlakov thank you for checking. It was the issue reported by @kdembler (I had fixed it already but forgot to redeploy 🤦). About the spam it's probably just Gmail seeing my personal address used to send transactional emails (nothing to do with the code). Please check again when you can. |
No description provided.