Skip to content
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

CSL-87: 1.15 CD What trading reasons does this site need a licence for? #61

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

jamiecarterHO
Copy link
Contributor

@jamiecarterHO jamiecarterHO commented Feb 6, 2025

What?

CSL-87 Adds trading reasons aggregator loop to the controlled drugs form

Summary page aggregates and parses options added via:

  • 'trading-reasons' page with drop-down select. Option data pulled from json doc due to length.
  • 'specify-trading-reasons' page forked to if above was 'other' allows user to add context to 'Other' selection

Behaviour around summary routing is copied from the already existent substances-summary.

Anything Else? (optional)

refactored earlier findChemical(value: string) utility to findArrayItemByValue(data: array, value: string) to allow it to be dynamic when finding an item in array array by its value prop (with array data structures assumed to have one).

Updated tests and usage of findChemical() in the precursor chemicals form to use new function.

Made final summary page for controlled drugs a full width page

Check list

  • I have reviewed my own pull request
  • I have written tests (if relevant)

Copy link
Collaborator

@dk4g dk4g left a comment

Choose a reason for hiding this comment

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

all good, just minor improvements suggestions

],
titleField: 'trading-reasons',
addStep: 'trading-reasons',
continueOnEdit: false,
Copy link
Collaborator

Choose a reason for hiding this comment

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

continueOnEdit is redundant here

utils/index.js Outdated
Comment on lines 88 to 90
* Using the form select value in a data array of items containing a value property
* Finds and returns the first matching array item from the array.
* From the found item other properties such as 'label' can be extracted.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* Using the form select value in a data array of items containing a value property
* Finds and returns the first matching array item from the array.
* From the found item other properties such as 'label' can be extracted.
* Finds and returns the first matching item from an array of objects based on a given value.
* The objects in the array should contain a 'value' property, and optionally other properties such as 'label'.

utils/index.js Outdated
Comment on lines 92 to 94
* @param {array} data - A json or js formatted array of objects each containing a label and value property.
* @param {string} valueToFind - The form value of the selected item to find in the array 'data'.
* @returns {object|undefined} - Containing all the details of the found item. Undefined if no item was found.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* @param {array} data - A json or js formatted array of objects each containing a label and value property.
* @param {string} valueToFind - The form value of the selected item to find in the array 'data'.
* @returns {object|undefined} - Containing all the details of the found item. Undefined if no item was found.
* @param {array} data - An array of objects, each containing at least a 'value' property.
* @param {string} valueToFind - The value to search for in the array.
* @returns {object|undefined} - The first object that matches the given value, or undefined if no match is found.

"specify-trading-reasons": {
"header": "Specify your trading reason"
"header": "Summary of trading reasons",
"add-another": "Add another",
Copy link
Collaborator

Choose a reason for hiding this comment

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

translation from buttons.json can be reused
"add-another": "Add another"

@jamiecarterHO jamiecarterHO requested a review from dk4g February 11, 2025 11:36
'change' is used in the final summary, using same name for all summary change buttons avoids fallback to HOF and is slightly safer
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants