DT-944: Fix chairperson edit DAC feature #2702
Conversation
rushtong
requested review from
pshapiro4broad and
cinyecai
and removed request for
a team
| const createdDaa = await DAA.createDaa(daaFileData, currentDac.dacId); | ||
| setCreatedDaa(createdDaa.data); | ||
| } else { | ||
| if (user.isAdmin) { | ||
| currentDac = await DAC.create(currentDac.name, currentDac.description, currentDac.email); |
There was a problem hiding this comment.
I'm having trouble following the different cases here. If there is a dacId, update the DAC, else if there is non broad DAA selected without a file throw an error, else if the user is an admin and there is DAA file data, create a DAC and a DAA for it, else if the user is an admin create a dac. This is pretty complex and not super clear, I think it could benefit from either comments or helper functions or something to make this flow easier to return to in the future.
There was a problem hiding this comment.
I'm trying to maintain the existing logic minus the chairperson restriction that existed before. I can take another stab at trying to refactor this, but my primary goal is bug fixing.
There was a problem hiding this comment.
I don't want to prevent progress/bug fixing here if this is too big of a lift, but component tests for this sort of complex logic might be helpful to confirm it is working as expected.
There was a problem hiding this comment.
Good idea ... let me see what I can do here.
There was a problem hiding this comment.
@raejohanek - Another way to look at this is to hide the white space in the PR. That should make the actual changes much easier to see.
| @@ -374,7 +371,6 @@ export default function EditDac(props) { | |||
| name="name" | |||
| className="form-control vote-input" | |||
| required={true} | |||
| disabled={props.location.state.userRole === CHAIRPERSON} | |||
There was a problem hiding this comment.
why were these disabled for chairpersons?
There was a problem hiding this comment.
I think it was an historic decision that doesn't make sense to me. I opted to remove it. Chairs should be able to update the values of their DAC.
| @@ -250,30 +248,37 @@ export default function ManageEditDac(props) { | |||
| return ( | |||
| isLoading ? | |||
| <Spinner/> : | |||
| <div className='container container-wide'> | |||
| <div className='row no-margin'> | |||
| <div className="container container-wide"> | |||
There was a problem hiding this comment.
usage of " and ' is pretty inconsistent. is " the standard?
There was a problem hiding this comment.
Good catch, single quotes are the standard - not sure why the linter didn't pick this up
|
I created https://broadworkbench.atlassian.net/browse/DT-959 to address the lack of unit tests so we can get this bug fix into the next release. |
Addresses
https://broadworkbench.atlassian.net/browse/DT-944
Summary
This PR makes a number of changes in function and style. The functional changes are intentional. As a business rule, we allow for Chairs to make changes to their own DACs in terms of both membership and other related details. There are also a number of minor style fixes.
Requires corresponding Consent PR: DataBiosphere/consent#2419
Testing Notes
envin your localconfig.jsonto beprodso that you're seeing what we would see in production.envconfig todevorstaging.Prod: New screen
Prod: Old screen
Non-prod: New screen
Non-prod: Old screen
Have you read Terra's Contributing Guide lately? If not, do that first.