-
-
Notifications
You must be signed in to change notification settings - Fork 192
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
Contract tool: MVP #1058
base: master
Are you sure you want to change the base?
Contract tool: MVP #1058
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.
It's looking good so far, just a few comments on things you're most likely already aware of.
|
||
/** | ||
* @function createContract | ||
* @description Get Contracts from the API |
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.
Incorrect description
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.
All the descriptions need updating.
Authorization: session.token, | ||
}; | ||
|
||
return Axios.patch(`/agreement`, contract, { headers }).then( |
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.
Is the /
before agreements
correct syntax and needed? Other calls do not have the slash.
@@ -98,7 +98,7 @@ export default { | |||
lon: capture.lon, | |||
gps_accuracy: capture.gps_accuracy, | |||
captured_at: capture.captured_at, | |||
note: capture.note ? capture.note : null, | |||
note: capture.note ? capture.note : ' ', // temporary measure because the api requires a non-empty string, but adding notes is not in the UX yet |
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.
Let's not push this change - we should get the constraint removed from the API instead, unless it's a deliberate change in behaviour.
* @name contractsTableMetaData | ||
* @description contains table meta data | ||
* @type {Object[]} | ||
* @param {string} contractsTableMetaData[].name - earning property used to get earning property value from earning object to display in table |
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.
Make sure you update references to earnings
setIsLoading(true); // show loading indicator when fetching data | ||
|
||
const { results, totalCount = 0 } = await getContractAgreements(fetchAll); | ||
console.log('results:', results, 'totalCount:', totalCount); |
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.
Usual comment about using loglevel
in place of console.log
// return; | ||
// } else { | ||
if ( | ||
filtersToSubmit[k] === 'all' || |
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 think this is a bug that's been carried over from the Earnings table – the phrase "all" could be a valid search term (e.g., part of a contract name) and shouldn't be used as a special value.
} | ||
|
||
async function getContractAgreements(fetchAll = false) { | ||
const filtersToSubmit = { ...filter }; |
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'd expect the Name
query to be case-insensitive and support partial matches, but this may be an API issue.
}} | ||
selectedRow={selectedContractAgreement} | ||
tableMetaData={contractsTableMetaData} | ||
activeFiltersCount={ |
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.
Pressing Reset does not clear the filter count and doesn't correctly reset the table. This may be a bug in the common component though.
export const AGREEMENT_TYPE = { | ||
all: 'all', | ||
grower: 'grower', | ||
nursury: 'nursury', |
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.
This should be spelled nursery
|
||
const defaultOrgList = userHasOrg | ||
? [ | ||
{ |
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.
Does all
make sense here?
Description
DONE:
TO DO:
Attaching Documents (document) to Contracts (via contract_document)
One fixed, generic Agreement (agreement) that can be applied to multiple Contracts
-- Need actual example.....
Capture <-> Contract linkage (treetracker.capture.contract_id)
-- Need to add field to capture table
-- Script to fill in the contract_id field based on connection to grower_account_id/worker_id and the time & location of capture?
Earnings <-> Contract linkage (earnings.earnings.contract_id)
-- Need to add field to earnings table?
-- Script to fill in the contract_id field based on connection to grower_account_id/worker_id alone?
create a Contract Query that will include...
-- the org name,
-- # of captures based on the grower_account,
-- and type of agreement
-- maybe also the agreement name, contractor name, and other information to make the table more readable?
fix the date query and other filters that are not currently allowed by the Contract and Agreement APIs
-- what do we want to be able to filter by?
add form to edit contracts and agreements
add tests
get better designs / UX and icons for the different sections
add other functionality: Species Agreement CRUD, Consolidation Rule CRUD
Issue(s) addressed
What kind of change(s) does this PR introduce?
Please check if the PR fulfills these requirements