-
Notifications
You must be signed in to change notification settings - Fork 43
Adjust logic for returning items from boards #132
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: master
Are you sure you want to change the base?
Conversation
|
|
||
| setResponse: (data: any) => { | ||
| mockRequest.mockResolvedValue(data); | ||
| setResponse: (data: any, onlyOnce = false) => { |
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.
How about having a separate function instead?
setResponseOnce: (data: any) => ...
| return JSON.parse(cv.value); | ||
| } catch { | ||
| return cv.value | ||
| return cv.value || null; |
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.
Why null as a default value and not an empty string?
| const aggregationsToBuild = input.aggregations!.slice(); | ||
|
|
||
| // select fullname of people when grouping by people columns | ||
| for(const peopleColumnId of input.groupBy?.filter(columnId => peopleColumnIds.includes(columnId)) ?? []) { |
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.
Wouldn't it be cleaner to have the filter logic inside the loop body as an if statement?
for(const peopleColumnId of (input.groupBy ?? [])) {
if (...) {
aggregationsToBuild.push(...)
}
}
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.
or event cleaner
const peopleAggregations = input.groupBy?
.filter(columnId => peopleColumnIds.includes(columnId)) ?? [])
.map(peopleColumnId => ({
function: AggregateSelectFunctionName.Label,
columnId: peopleColumnId,
})
const selectElements = [...input.aggregations, ...peopleAggregations].map(...)| return { content: `Board with id ${input.boardId} not found or you don't have access to it.` }; | ||
| } | ||
|
|
||
| const peopleColumnIds = board.columns?.filter((column) => column?.type === NonDeprecatedColumnType.People)?.map((column) => column!.id) ?? []; |
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 got to ask:
Now you added the ability to group by specifically person column.
Why we are adding this? Is it a customer request?
Second, there are different columns that are also worth to group (if needed). For example: status column or date column.
So the grouping ability is going to be a bit more generic in the future? Or currently we are only going to group by people?
Adjust board_insights to get people names when grouping by people column
Made sure following columns are supported: people, status, text, number, dropdown, formula, board relation
Marked mirror column as not supported
Example 1: "People" column has fullname in "text" field, while "value" includes person id
Outcome:
LLM now requires 1 less tool invocation to return human-friendly data. It also fixes issue when less-smart/non-reasoning LLMs were sometimes just pasting user_ids as "task owners" or label ids.
BEFORE (2 calls or 1 call and non-human-friendly output with ids):


AFTER CHANGE (1 call & human-friendly output):
