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

feat: Enhancement in notifications #11398

Merged
merged 8 commits into from
Sep 3, 2020

Conversation

deepeshgarg007
Copy link
Member

@deepeshgarg007 deepeshgarg007 commented Sep 2, 2020

Ability to send notifications to assignees

Added a check to send emails to all assignees
Screenshot 2020-09-02 at 3 59 17 PM

Send notification based on child table field

Now child table fields also show up in the receiver_by_document_field in the recipients child table
Screenshot 2020-09-02 at 4 01 28 PM

@prssanna
Copy link
Contributor

prssanna commented Sep 3, 2020

@deepeshgarg007 should we add assigned users to the Receiver by Document Field options instead of using a new field to send to all Assignees?

(df.options == 'User' && df.fieldtype == 'Link')
? get_select_options(df, d.fieldname) : null;
});
} else {
Copy link
Member

Choose a reason for hiding this comment

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

what is this additional thing?

Copy link
Member Author

Choose a reason for hiding this comment

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

This adds fields with options as "User" and "Email" from child tables in the select dropdown in the recipients table same way we do for fields on parent level

Copy link
Member

Choose a reason for hiding this comment

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

ok can you add comments, because it is hard to read the logic.

also add comments in other places where you think it will be helpful in review ;-)

@deepeshgarg007
Copy link
Member Author

deepeshgarg007 commented Sep 3, 2020

@deepeshgarg007 should we add assigned users to the Receiver by Document Field options instead of using a new field to send to all Assignees?

Technically it(Assignees) is not a doc field so thought to keep it outside as a separate field and not under the Receiver By Document field. Is there any docfield from where I can get all the assignees. If yes then let me know as I am not aware of it.

We also have conditions at the parent level as well so there was no incentive to put it in the child table. Here a separate field makes it clear to the user what checking the field will do, in child table it might be a bit unclear. If moving the option in child table gives user more control and I have missed anything let me know, will move it to the child table then

@prssanna
Copy link
Contributor

prssanna commented Sep 3, 2020

@deepeshgarg007 we have doc._assign. I just thought that grouping all the recipient options together would make more sense UX-wise, but yeah this is a more clear implementation. Also the condition in the child table gives the user the power to set specific conditions for different sets of recipients (but I'm not sure how much that is actually used).

@mergify mergify bot merged commit 1ce1019 into frappe:develop Sep 3, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 31, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants