Add engagement flags #78
Conversation
src/firestore/app/run.ts
Outdated
| const engagementObj = engagementFlags.reduce((acc: { [x: string]: unknown }, flag) => { | ||
| acc[flag] = true; | ||
| return acc; | ||
| }, {}); | ||
|
|
||
| if (!this.aborted) { | ||
| return updateDoc(this.runRef, { engagementFlags: engagementObj, reliable: !markAsUnreliable }); | ||
| } else { | ||
| throw new Error('Run has already been aborted.'); |
There was a problem hiding this comment.
i am not sure I follow this. what is engagementObj? can you explain it a bit? thanks!
| if (!this.aborted) { | ||
| return updateDoc(this.runRef, { engagementFlags: engagementObj, reliable: markAsReliable }); |
There was a problem hiding this comment.
As it's written right now, this will overwrite any engagement flags that are already there. I think that might be okay but I just want to make sure that it's explicit. Can you add a comment to the docstring of both this method and the appkit method to let the user know that they will be overwriting any existing engagement flags for this run? It will then be the ROAR-app developer's responsibility to keep track of any previously assigned engagement flags.
|
Looks good to me. I made a small comment requesting docstring changes. But once that is done then this is good to go. Thanks @AnyaWMa and @lucasxsong! |
No description provided.