Skip to content

fix: use correct instrument decision for fap meetings and management decision spreadsheets - #1901

Open
GrantDLS wants to merge 3 commits into
developfrom
multi-instrument-proposal-fap-spreadsheet
Open

GrantDLS wants to merge 3 commits into
developfrom
multi-instrument-proposal-fap-spreadsheet

Conversation

@GrantDLS

Copy link
Copy Markdown
Contributor

Description

FAP meeting decisions are stored per proposal and instrument. The spreadsheet exports previously selected the first decision for a proposal, causing all instruments for a multi-instrument proposal to display the same recommendation and comments.

Changes

Updated both the FAP results and management decision spreadsheet exports to match each decision by proposal, fap and instrument.

Tests included/Docs Updated?

New tests added for management decision export.

@GrantDLS
GrantDLS requested a review from a team as a code owner October 16, 2026 10:19
@GrantDLS
GrantDLS requested review from EmmaHancock and removed request for a team October 16, 2026 10:19
@zacharyjhankin
zacharyjhankin self-requested a review October 16, 2026 10:22
@zacharyjhankin

Copy link
Copy Markdown
Collaborator

Tested it locally and it works to fix the bug.

fapMeetingDecision.recommendation
]
: null,
fapMeetingExComment: fapMeetingDecision?.commentForUser

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think there is a small mistake here that the empty string is falsy in JS, so would return null. But we probably want to stick to returning the empty string.

We'd want to use this check:

fapMeetingInComment: fapMeetingDecision?.commentForManagement != null
? stripHtml(fapMeetingDecision.commentForManagement)
: null,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think the empty string is good to go into the docs as compared to

fapMeetingData?.[0]?.commentForUser
? stripHtml(fapMeetingData[0].commentForUser)
fapMeetingDecision?.commentForUser
? stripHtml(fapMeetingDecision.commentForUser)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Could do this too: fapMeetingInUserComment: fapMeetingDecision?.commentForUser != null
? stripHtml(fapMeetingDecision.commentForUser)
: '',

@zacharyjhankin zacharyjhankin left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

looks good

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants