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.

Sign up for GitHub

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

DT-944: Fix chairperson edit DAC feature #2702

Merged
merged 3 commits into from
Nov 1, 2024
Merged

DT-944: Fix chairperson edit DAC feature #2702

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a254327
feat: fix chairperson edit ability
rushtong Oct 30, 2024
d8d006d
feat: repeat changes for manage dac DAA version
rushtong Oct 30, 2024
dcd3d9a
feat: PR feedback - quote consistency
rushtong Oct 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 30 additions & 36 deletions src/pages/manage_dac/EditDac.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,10 @@ import { Styles } from '../../libs/theme';
import DUOSUniformDataAccessAgreement from '../../assets/DUOS_Uniform_Data_Access_Agreement.pdf';
import PublishIcon from '@mui/icons-material/Publish';
import { UploadDaaModal } from '../../components/modals/UploadDaaModal';
import {Storage} from '../../libs/storage';

export const CHAIR = 'chair';
export const MEMBER = 'member';
const CHAIRPERSON = 'Chairperson';
const ADMIN = 'Admin';

export default function EditDac(props) {
const [state, setState] = useState({
Expand Down Expand Up @@ -83,44 +82,42 @@ export default function EditDac(props) {

const okHandler = async (event) => {
event.preventDefault();

const user = Storage.getCurrentUser();
let currentDac = state.dac;
if (state.dirtyFlag) {
if (props.location.state.userRole === ADMIN) {
if (dacId !== undefined) {
await DAC.update(currentDac.dacId, currentDac.name, currentDac.description, currentDac.email);
if (dacId !== undefined) {
await DAC.update(currentDac.dacId, currentDac.name, currentDac.description, currentDac.email);
} else {
if (daaFileData === null && selectedDaa.daaId !== broadDaa.daaId) {
handleErrors('Please select either the default agreement or upload your own agreement before saving.');
return;
} else if (user.isAdmin && daaFileData !== null && selectedDaa === undefined) {
currentDac = await DAC.create(currentDac.name, currentDac.description, currentDac.email);
const createdDaa = await DAA.createDaa(daaFileData, currentDac.dacId);
setCreatedDaa(createdDaa.data);
} else {
if (daaFileData === null && selectedDaa.daaId !== broadDaa.daaId) {
handleErrors('Please select either the default agreement or upload your own agreement before saving.');
return;
} else if (daaFileData !== null && selectedDaa === undefined) {
currentDac = await DAC.create(currentDac.name, currentDac.description, currentDac.email);
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);
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea ... let me see what I can do here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

Screenshot 2024-10-31 at 1 03 06 PM

}
}
}

// Order here is important. Since users cannot have multiple roles in the
// same DAC, we have to make sure we remove users before re-adding any
// back in a different role.
// Chairs are a special case since we cannot remove all chairs from a DAC
// so we handle that case first.
const ops0 = state.chairIdsToAdd.map(id => () => DAC.removeDacMember(currentDac.dacId, id));
const ops1 = state.memberIdsToRemove.map(id => () => DAC.removeDacMember(currentDac.dacId, id));
const ops2 = state.chairIdsToAdd.map(id => () => DAC.addDacChair(currentDac.dacId, id));
const ops3 = state.chairIdsToRemove.map(id => () => DAC.removeDacChair(currentDac.dacId, id));
const ops4 = state.memberIdsToAdd.map(id => () => DAC.addDacMember(currentDac.dacId, id));
const ops5 = newDaaId !== null && selectedDaa !== undefined ? [() => DAA.addDaaToDac(newDaaId, currentDac.dacId)] : [];
const allOperations = ops0.concat(ops1, ops2, ops3, ops4, ops5);
const responses = await PromiseSerial(allOperations);
const errorCodes = ld.filter(responses, r => JSON.stringify(r) !== '200' && JSON.stringify(r.status) !== '201');
if (!ld.isEmpty(errorCodes)) {
handleErrors('There was an error saving DAC information. Please verify that the DAC is correct by viewing the current information.');
} else {
closeHandler();
}
// Order here is important. Since users cannot have multiple roles in the
// same DAC, we have to make sure we remove users before re-adding any
// back in a different role.
// Chairs are a special case since we cannot remove all chairs from a DAC
// so we handle that case first.
const ops0 = state.chairIdsToAdd.map(id => () => DAC.removeDacMember(currentDac.dacId, id));
const ops1 = state.memberIdsToRemove.map(id => () => DAC.removeDacMember(currentDac.dacId, id));
const ops2 = state.chairIdsToAdd.map(id => () => DAC.addDacChair(currentDac.dacId, id));
const ops3 = state.chairIdsToRemove.map(id => () => DAC.removeDacChair(currentDac.dacId, id));
const ops4 = state.memberIdsToAdd.map(id => () => DAC.addDacMember(currentDac.dacId, id));
const ops5 = newDaaId !== null && selectedDaa !== undefined ? [() => DAA.addDaaToDac(newDaaId, currentDac.dacId)] : [];
const allOperations = ops0.concat(ops1, ops2, ops3, ops4, ops5);
const responses = await PromiseSerial(allOperations);
const errorCodes = ld.filter(responses, r => JSON.stringify(r) !== '200' && JSON.stringify(r.status) !== '201');
if (!ld.isEmpty(errorCodes)) {
handleErrors('There was an error saving DAC information. Please verify that the DAC is correct by viewing the current information.');
} else {
closeHandler();
}
Expand Down Expand Up @@ -374,7 +371,6 @@ export default function EditDac(props) {
name="name"
className="form-control vote-input"
required={true}
disabled={props.location.state.userRole === CHAIRPERSON}
Copy link
Contributor

Choose a reason for hiding this comment

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

why were these disabled for chairpersons?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

/>
</div>
</div>
Expand All @@ -389,7 +385,6 @@ export default function EditDac(props) {
name="description"
className="form-control vote-input"
required={true}
disabled={props.location.state.userRole === CHAIRPERSON}
/>
</div>
</div>
Expand All @@ -405,7 +400,6 @@ export default function EditDac(props) {
name="email"
className="form-control vote-input"
required={true}
disabled={props.location.state.userRole === CHAIRPERSON}
/>
</div>
</div>
Expand Down
Loading
Loading