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

Add option to copy new value to form in edit profile workbench #10885

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add option to copy new value to form in edit profile workbench #10885

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
13 changes: 12 additions & 1 deletion app/webpacker/components/Panel/pages/EditPersonPage/EditPersonForm.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,12 @@ import UtcDatePicker from '../../../wca/UtcDatePicker';
import CountrySelector from '../../../CountrySelector/CountrySelector';
import GenderSelector from '../../../GenderSelector/GenderSelector';

export default function EditPersonForm({ wcaId, onSuccess, showDestroyButton = false }) {
export default function EditPersonForm({
wcaId,
onSuccess,
showDestroyButton = false,
defaultValues,
}) {
const {
data: personFetchData, loading, error: personError,
} = useLoadedData(
Expand Down Expand Up @@ -85,6 +90,12 @@ export default function EditPersonForm({ wcaId, onSuccess, showDestroyButton = f
}, { method: 'PUT' }, (error) => setResponse({ success: false, message: `${error}` }));
};

useEffect(() => {
if (defaultValues) {
setEditedUserDetails((prev) => ({ ...prev, ...defaultValues }));
}
}, [defaultValues]);
Comment on lines +93 to +97
Copy link
Member

Choose a reason for hiding this comment

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

This is not what useEffect was designed for. Is there really no other way?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was chatting with Daniel and we came up with this solution - I can try to find a different solution, but there is a high chance that useEffect is necessary here

Copy link
Member

Choose a reason for hiding this comment

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

One way I can think of where we can avoid useEffect is - pass the editedUserDetails and setEditedUserDetails from the parent component. But since this form is used in two different places, I'm bit reluctant to use that way and will be more inclined towards using useEffect.

Copy link
Member

Choose a reason for hiding this comment

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

After thinking again, I got one more solution - for this I would like to rename defaultValues to forcedValues. The forcedValues will initially be {}.

When a field is populated, it will become like {name: "John Doe"}.

Now in <Form.Input.../>, we can have the value as {forcedValues?.name || <whatever is already there>} and disabled will be {forcedValues?.name || <whatever is already there>}. This way, if there is a forced value, that will be having highest priority and user won't be able to edit it as well.

While calling the API, instead of sending person as editedUserDetails, we might have to send {...editedUserDetails, ...forcedValues}.

This might be complicated, I'm not sure. @gregorbg WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

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

One way I can think of where we can avoid useEffect is - pass the editedUserDetails and setEditedUserDetails from the parent component. But since this form is used in two different places, I'm bit reluctant to use that way and will be more inclined towards using useEffect.

I've also considered this but I think this would make the component much more complicated and harder to use in other places, so I agree with you.

I have no strong preference regarding your second comment, both solutions would probably work but the second one looks more complicated than mine. Let's wait for Gregor's opinion, if you guys think that that would be better, I'm more than happy to implement it in that way.


if (loading || saving) return <Loading />;
if (personError) return <Errored />;

Expand Down
30 changes: 26 additions & 4 deletions app/webpacker/components/Tickets/TicketWorkbenches/EditPersonTicketWorkbench.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import React from 'react';
import { Header, Message, Table } from 'semantic-ui-react';
import React, { useState } from 'react';
import {
Button, Header, Message, Table,
} from 'semantic-ui-react';
import EditPersonForm from '../../Panel/pages/EditPersonPage/EditPersonForm';
import useSaveAction from '../../../lib/hooks/useSaveAction';
import { actionUrls } from '../../../lib/requests/routes.js.erb';
Expand Down Expand Up @@ -30,7 +32,7 @@ function EditPersonValidations({ ticketDetails }) {
));
}

function EditPersonRequestedChangesList({ requestedChanges }) {
function EditPersonRequestedChangesList({ requestedChanges, copyToForm }) {
return (
<>
<Header as="h3">Requested changes</Header>
Expand All @@ -40,14 +42,23 @@ function EditPersonRequestedChangesList({ requestedChanges }) {
<Table.HeaderCell>Field</Table.HeaderCell>
<Table.HeaderCell>Old value</Table.HeaderCell>
<Table.HeaderCell>New value</Table.HeaderCell>
<Table.HeaderCell>Actions</Table.HeaderCell>
</Table.Row>
</Table.Header>
<Table.Body>
{requestedChanges?.map((change) => (
<Table.Row>
<Table.Row key={change.field_name}>
<Table.Cell>{I18n.t(`activerecord.attributes.user.${change.field_name}`)}</Table.Cell>
<Table.Cell>{formatField(change.field_name, change.old_value)}</Table.Cell>
<Table.Cell>{formatField(change.field_name, change.new_value)}</Table.Cell>
<Table.Cell>
<Button onClick={() => copyToForm(change.field_name, change.new_value)}>
Copy to form
</Button>
<Button onClick={() => copyToForm(change.field_name, change.old_value)}>
Undo
</Button>
</Table.Cell>
</Table.Row>
))}
</Table.Body>
Expand All @@ -59,6 +70,11 @@ function EditPersonRequestedChangesList({ requestedChanges }) {
function EditPersonTicketWorkbenchForWrt({ ticketDetails, actingStakeholderId, sync }) {
const { ticket } = ticketDetails;
const { save, saving } = useSaveAction();
const [defaultValues, setDefaultValues] = useState(
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to prefill this with default value? Instead if we have it empty and populate whenever the button is clicked, won't this work?

ticket.metadata?.tickets_edit_person_fields.map((change) => ({
[change.field_name]: change.old_value,
})).reduce((acc, obj) => ({ ...acc, ...obj }), {}),
);

const closeTicket = () => {
save(
Expand All @@ -72,6 +88,10 @@ function EditPersonTicketWorkbenchForWrt({ ticketDetails, actingStakeholderId, s
);
};

const copyToForm = (field, value) => {
setDefaultValues({ ...defaultValues, [field]: value });
};

if (saving) return <Loading />;

return (
Expand All @@ -81,10 +101,12 @@ function EditPersonTicketWorkbenchForWrt({ ticketDetails, actingStakeholderId, s
/>
<EditPersonRequestedChangesList
requestedChanges={ticket.metadata?.tickets_edit_person_fields}
copyToForm={copyToForm}
/>
<EditPersonForm
wcaId={ticket.metadata.wca_id}
onSuccess={closeTicket}
defaultValues={defaultValues}
/>
</>
);
Expand Down