-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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 new sample that is for a Google Workspace Add-on that adds sentim… #496
Conversation
…ent analysis capabilities to Gmail.
🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use -- conventional-commit-lint bot |
"contextualTriggers": [ | ||
{ | ||
"unconditional": {}, | ||
"onTriggerFunction": "onGmailMessage" |
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.
Doesn't appear to be implemented, just the generic home page. Remove it not used.
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.
Checked the dev docs and I can delete the entire conextualTriggers object.
const regex = new RegExp('N'); | ||
let currentPrediction; | ||
|
||
for (let i = 0 ; i < msgs.length; i++) { |
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.
Nit: Prefer for..of or arr.forEach()
Examples:
for(let msg of msgs) {
...
or
msgs.forEach(msg => {
...
});
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.
My personal preference is the forEach method when looping over an array.
But the traditional for loop is significantly quicker. I've tested it and I think we should keep the code as is to make sure there aren't any timeouts.
gmail-sentiment-analysis/Gmail.gs
Outdated
* Gets the last 10 threads in the inbox and the corresponding messages. | ||
* Fetches the label that should be applied to negative messages. | ||
* The processSentiment is called on each message | ||
* and testet with RegExp to check for a negative answer from the model |
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.
nit: typo "testet" -> "tested"
gmail-sentiment-analysis/Code.gs
Outdated
* @return {CardService.Card} The card to show to the user. | ||
*/ | ||
function onHomepage(e) { | ||
if(e.hostApp =="gmail"){ |
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.
Unnecessary condition. If you want it to only show in Gmail, move the homepage trigger in the manifest from common to the Gmail config
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.
Will remove and change manifest file accordingly.
gmail-sentiment-analysis/Gmail.gs
Outdated
for (let j = 0; j < msgs[i].length; j++) { | ||
let emailText = msgs[i][j].getPlainBody(); | ||
currentPrediction = processSentiment(emailText); | ||
if(regex.test(currentPrediction)){ |
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.
Not a great way to check (and regex will pass with any capital N anywhere in the response.)
When I was reading the regex, it waasn't clear why the pattern was "N"
Suggest moving the string matching into processSentiment and, if you only care about whether it is negative or not, return a boolean or at least a constant that is clearer to compare against in this block.
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.
Holy smokes, I spent hours on this line of code!
It seems as if there's a hidden character (maybe a new line?) because the simple check currendPrediction === "FALSE" always was false. So the label was never applied even if the response from Vertex AI was "FALSE".
…ent analysis capabilities to Gmail.
Description
This is a new sample that creates a Google Workspace Add-ons that extends Gmail with sentiment analysis capabilities.
Fixes # (issue)
Is it been tested?
Checklist