feat: diamond specific FAP spreadsheet columns#1656
Conversation
… to avoid needing to use positional arguments.
GrantDLS
changed the title
GrantDLS
changed the title
GrantDLS
changed the title
There was a problem hiding this comment.
Do you ever run the cypress tests in DEPENDENCY_CONFIG === 'dls'? if not i don't think we need this file?
zacharyjhankin
left a comment
zacharyjhankin
left a comment
There was a problem hiding this comment.
There is a bug in the code where the comments to users and the comments to mangement are mixed up
| 'Institution', | ||
| 'Instrument', | ||
| 'Instrument available Time', | ||
| 'TA Recommended Time', |
There was a problem hiding this comment.
This needs to be changed to "Technical Review Recommended Time", as confirmed with Marcos. This is to match the UI as "TA" makes no sense.
|
I think Average Score can't be zero when there is no data, it should be . At least for our implementation of it |
| nullFieldHelper(row.instrName), | ||
| nullFieldHelper(row.instrAvailTime), | ||
| nullFieldHelper(row.techReviewTimeAllocation), | ||
| nullFieldHelper(row.fapTimeAllocation ?? row.techReviewTimeAllocation), |
There was a problem hiding this comment.
Surely this is not right, they are completely different values
| const userDataSource = container.resolve<UserDataSource>( | ||
| Tokens.UserDataSource | ||
| ); | ||
| const pi = proposerId |
There was a problem hiding this comment.
I don't like pi here, its not clear if they are a principleInvestigator, or a propserID. I think they are the same thing. If I am right should be principleInvestigator
| instrumentAvailabilityTime: number; | ||
| fapTimeAllocation: number | null; | ||
| proposalTitle: string; | ||
| proposalId: number | null; |
There was a problem hiding this comment.
Does this need to be nullable?
| }; | ||
| } | ||
|
|
||
| export function populateDLSRow(row: DLSFapRowObj): (string | number)[] { |
There was a problem hiding this comment.
We don't need to pass the DLSFapRowObj type here because you're not even using the new instrumentRequestedTime value. That is being used here callFapDLSPopulateRow
4 participants
Description
We want to include some different fields in the FAP spreadsheet download, so this change adds in the new fields and renames some others. It uses dependency injection to introduce this functionality so that existing configurations are unaffected.
Motivation and Context
The
getDataRowfunction was changed from positional parameters to a singleFapDataRowInputobject because the function is selected through dependency injection and different facility implementations require different inputs.The facility-specific implementations require different subsets of proposal data, and the positional API became fragile as new values such as the instrument ID were introduced. A named object prevents argument-order errors, provides a stable interface across implementations, and allows each facility to consume only the fields it requires.
The reason this worked before is because the function was being called with 14 parameters. The default implementation only needed 11 params and so would just ignore the additional 3. Following this same approach meant that the DLS implementation wasn't able to only accept 12 params - it needed to accept the extra three from the STFC implementation, plus the extra instrument ID that was needed.
How Has This Been Tested
Extra cypress fixtures have been added to test the inclusion of the new columns and the renamed ones.