Skip to content

fix: add new poll option only when all of the current options are filled #1532

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

Merged
merged 5 commits into from
May 19, 2025
Merged

fix: add new poll option only when all of the current options are filled #1532

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8864c7a
fix: add new poll option only when all of the current options are filled
khushal87 May 12, 2025
01ed3cf
fix: add new poll option only when all of the current options are filled
khushal87 May 12, 2025
103be13
fix: refine the implementation
khushal87 May 12, 2025
d2ed8ee
test: refine tests
khushal87 May 12, 2025
ce605bd
test: type in assertion
khushal87 May 13, 2025
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
16 changes: 9 additions & 7 deletions src/messageComposer/middleware/pollComposer/state.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,24 +116,26 @@ export const pollCompositionStateProcessors: Partial<
const { index, text } = value;
const prevOptions = data.options || [];

const shouldAddEmptyOption =
prevOptions.length < MAX_POLL_OPTIONS &&
(!prevOptions || (prevOptions.slice(index + 1).length === 0 && !!text));

const shouldRemoveOption =
prevOptions && prevOptions.slice(index + 1).length > 0 && !text;

const optionListHead = prevOptions.slice(0, index);
const optionListTail = shouldAddEmptyOption
? [{ id: generateUUIDv4(), text: '' }]
: prevOptions.slice(index + 1);
const optionListTail = prevOptions.slice(index + 1);

const newOptions = [
...optionListHead,
...(shouldRemoveOption ? [] : [{ ...prevOptions[index], text }]),
...optionListTail,
];

const shouldAddNewOption =
prevOptions.length < MAX_POLL_OPTIONS &&
!newOptions.some((option) => !option.text.trim());

if (shouldAddNewOption) {
newOptions.push({ id: generateUUIDv4(), text: '' });
}

return { options: newOptions };
},
};
Expand Down
55 changes: 52 additions & 3 deletions test/unit/MessageComposer/middleware/pollComposer/state.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,12 +182,14 @@ describe('PollComposerStateMiddleware', () => {
expect(result.status).toBeUndefined;
});

it('should add a new empty option when the last option is filled', async () => {
it('should add a new empty option when the all the options are filled', async () => {
const stateMiddleware = setup();
const result = await stateMiddleware.handlers.handleFieldChange(
setupHandlerParams({
nextState: { ...getInitialState() },
Copy link
Contributor

Choose a reason for hiding this comment

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

The next state has one empty option and the previous state has 3 options. That is inconsistent.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe you could keep this test as it was and add a new tests that:

  1. simulates the scenario of moving an option to a different position
  2. simulates the scenario of providing a value to a previously empty non-last option

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 have pushed the suggestions. Please let me know what you think. Thanks 😄

previousState: { ...getInitialState() },
previousState: {
...getInitialState(),
},
targetFields: {
options: {
index: 0,
Expand All @@ -199,7 +201,54 @@ describe('PollComposerStateMiddleware', () => {

expect(result.state.nextState.data.options.length).toBe(2);
expect(result.state.nextState.data.options[0].text).toBe('Option 1');
expect(result.state.nextState.data.options[1].text).toBe('');
expect(result.state.nextState.data.options[1].text).toEqual('');

expect(result.status).toBeUndefined;
});

it('should reorder options and add a new empty option when all the options are filled', async () => {
const stateMiddleware = setup();

const reOrderedOptions = [
{
id: 'option-2',
text: '',
},
{
id: 'option-1',
text: 'Option 1',
},
];

const result = await stateMiddleware.handlers.handleFieldChange(
setupHandlerParams({
nextState: {
...getInitialState(),
data: {
...getInitialState().data,
options: reOrderedOptions,
},
},
previousState: {
...getInitialState(),
data: {
...getInitialState().data,
options: reOrderedOptions,
},
},
targetFields: {
options: {
index: 0,
text: 'Option 2',
},
},
}),
);

expect(result.state.nextState.data.options.length).toBe(3);
expect(result.state.nextState.data.options[0].text).toBe('Option 2');
expect(result.state.nextState.data.options[1].text).toBe('Option 1');
expect(result.state.nextState.data.options[2].text).toEqual('');
expect(result.status).toBeUndefined;
});

Expand Down
Loading