-
Notifications
You must be signed in to change notification settings - Fork 2
Friendlier per-owner subtasks #34
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
base: main
Are you sure you want to change the base?
Conversation
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.
Thanks for doing this! I left some comments - but nothing major really, so I won't block approving.
@nshuba in the postmortem you mentioned wanting to have another look at things checked in since you reviewed. Let me know if there is anything else you'd like me to do on this before merging. |
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.
Looks good. Please fix the part where we might assign the same pixel to multiple people, the rest is minor. Feel free to merge after that.
pixelPhrase += ` | ||
|
||
New to these reports ? See <a href="${INSTRUCTIONS_TASK_URL}">View task</a>`; |
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.
Would this not work with Asana APIs?
pixelPhrase += ` | |
New to these reports ? See <a href="${INSTRUCTIONS_TASK_URL}">View task</a>`; | |
pixelPhrase += `\nNew to these reports ? See <a href="${INSTRUCTIONS_TASK_URL}">View task</a>`; |
|
||
// Write thisOwnersPixelsWithErrors to a temporary file | ||
const tempFilePath = path.join(dirPath, `pixel_errors_${owner}.json`); | ||
fs.writeFileSync(tempFilePath, JSON.stringify(ownersPixelData, null, 2)); |
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.
Minor: our prettier
standard is 4 spaces
if (pixel.owners) { | ||
pixel.owners.forEach((owner) => ownersSet.add(owner)); | ||
pixel.owners.forEach((owner) => { |
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.
Won't this assign the same pixel more than once if there are multiple owners? I'd just assign to the first owner. Otherwise we risk 2+ people spending their time fixing the same thing
No description provided.