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.

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

chore: Rahulbarwal/hotfix-v1.53/select-widget-handling #38259

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
50 commits
Select commit Hold shift + click to select a range
462db00
Merge pull request #36723 from appsmithorg/release
btsgh Oct 7, 2024
524d400
Merge pull request #36742 from appsmithorg/release
btsgh Oct 8, 2024
5c7a468
Merge pull request #36792 from appsmithorg/release
btsgh Oct 10, 2024
662df2d
chore: Supress the failure in case DB url is not found in CI (#36796)
abhvsn Oct 10, 2024
43b4737
chore: Supress the failure in case DB url is not found in CI
abhvsn Oct 10, 2024
7649de5
Merge pull request #36833 from appsmithorg/release
yatinappsmith Oct 11, 2024
cf75cf5
Merge pull request #36850 from appsmithorg/release
btsgh Oct 14, 2024
b086761
chore: Use Lettuce client to run clear keys across Redis nodes (#36862)
nidhi-nair Oct 14, 2024
23d5834
Merge pull request #36867 from appsmithorg/cp/20241014-1
nidhi-nair Oct 14, 2024
203b653
Merge pull request #36928 from appsmithorg/release
btsgh Oct 17, 2024
0a66486
Merge pull request #36962 from appsmithorg/release
btsgh Oct 18, 2024
c4e4ce3
Merge pull request #36985 from appsmithorg/release
btsgh Oct 21, 2024
6f7d58c
Merge pull request #37032 from appsmithorg/release
btsgh Oct 23, 2024
c3f8c5d
Merge pull request #37050 from appsmithorg/release
btsgh Oct 24, 2024
f170b92
Merge pull request #37082 from appsmithorg/release
btsgh Oct 25, 2024
19ffda1
Merge pull request #37113 from appsmithorg/release
btsgh Oct 28, 2024
67123fa
Merge pull request #37132 from appsmithorg/release
yatinappsmith Oct 30, 2024
5367d23
Merge pull request #37215 from appsmithorg/release
yatinappsmith Nov 5, 2024
7c3c28d
Merge pull request #37251 from appsmithorg/release
btsgh Nov 6, 2024
519ce9a
Merge pull request #37315 from appsmithorg/release
btsgh Nov 11, 2024
5811d43
Merge pull request #37332 from appsmithorg/release
btsgh Nov 12, 2024
ed4b37f
Merge pull request #37407 from appsmithorg/release
btsgh Nov 15, 2024
fc589d9
Merge pull request #37433 from appsmithorg/release
btsgh Nov 18, 2024
c3cb5ef
Merge pull request #37552 from appsmithorg/release
btsgh Nov 19, 2024
f041fcb
Merge pull request #37613 from appsmithorg/release
btsgh Nov 21, 2024
2613b67
Merge pull request #37645 from appsmithorg/release
btsgh Nov 22, 2024
9eb9e3d
Merge pull request #37669 from appsmithorg/release
yatinappsmith Nov 25, 2024
455abf8
fix: Signup from OAuth not being detected correctly (#37697)
sharat87 Nov 26, 2024
9a285aa
fix: Super user cache eviction when user is added via env variable (#…
abhvsn Nov 28, 2024
6c5fc61
fix: Super user cache eviction when user is added via env variable (#…
abhvsn Nov 28, 2024
4df3d45
chore: Google sheet shared drive support added behind a flag (#37776)
Nov 28, 2024
8a1a287
chore: action execution solution test fixed (#37793)
Nov 28, 2024
c5efa91
Merge pull request #37792 from appsmithorg/chore/cherry-pick-gs-share…
nidhi-nair Nov 28, 2024
a4c8a02
Merge pull request #37842 from appsmithorg/release
btsgh Nov 29, 2024
5ef0e94
Merge pull request #37866 from appsmithorg/release
btsgh Dec 2, 2024
a8e3d71
fix: Broken UI on JS module instance in the side by side view mode (#…
ankitakinger Dec 3, 2024
8d910dd
Merge pull request #37913 from appsmithorg/chore/cherry-pick-js-modul…
btsgh Dec 3, 2024
1edecf1
fix: fixing usage pulse for anon user (#37940)
brayn003 Dec 4, 2024
7930bec
Merge pull request #37952 from appsmithorg/chore/usage-pulse-cherry-pick
btsgh Dec 5, 2024
54c8e78
Merge pull request #38005 from appsmithorg/release
btsgh Dec 6, 2024
5f9f0fb
Merge pull request #38036 from appsmithorg/release
btsgh Dec 9, 2024
299ce06
fix: Only updating the required fields in User while generating usage…
trishaanand Dec 9, 2024
2199333
Merge pull request #38047 from appsmithorg/cherry-pick/idToken-missing
trishaanand Dec 9, 2024
7c21b1c
Merge pull request #38095 from appsmithorg/release
yatinappsmith Dec 11, 2024
ae9e006
fixing the client build failure on prod
ankitakinger Dec 11, 2024
76fc6f5
Merge pull request #38109 from appsmithorg/chore/cherry-picking-fix
mohanarpit Dec 11, 2024
6689edd
Merge pull request #38124 from appsmithorg/release
btsgh Dec 12, 2024
f7b0472
Merge pull request #38146 from appsmithorg/release
btsgh Dec 13, 2024
16b5d20
Merge pull request #38181 from appsmithorg/release
btsgh Dec 16, 2024
4453206
fix: Enhance SelectWidget label and value handling logic (#38254)
rahulbarwal Dec 19, 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
38 changes: 36 additions & 2 deletions app/client/src/widgets/SelectWidget/widget/derived.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,16 +5,50 @@ export default {
values = [],
sourceData = props.sourceData || [];

const processOptionArray = (optionArray, sourceData) => {
if (!sourceData.length) return [];

const allEqual = optionArray.every((item, _, arr) => item === arr[0]);
const keyExistsInSource = optionArray[0] in sourceData[0];

return allEqual && keyExistsInSource
? sourceData.map((d) => d[optionArray[0]])
: optionArray;
};
Comment on lines +8 to +17
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add null checks and safer property access

The function needs defensive programming for edge cases.

Apply these improvements:

 const processOptionArray = (optionArray, sourceData) => {
-  if (!sourceData.length) return [];
+  if (!Array.isArray(sourceData) || !sourceData.length || !sourceData[0]) return [];
 
   const allEqual = optionArray.every((item, _, arr) => item === arr[0]);
-  const keyExistsInSource = optionArray[0] in sourceData[0];
+  const keyExistsInSource = Object.hasOwn(sourceData[0], String(optionArray[0]));
 
   return allEqual && keyExistsInSource
     ? sourceData.map((d) => d[optionArray[0]])
     : optionArray;
 };
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const processOptionArray = (optionArray, sourceData) => {
if (!sourceData.length) return [];
const allEqual = optionArray.every((item, _, arr) => item === arr[0]);
const keyExistsInSource = optionArray[0] in sourceData[0];
return allEqual && keyExistsInSource
? sourceData.map((d) => d[optionArray[0]])
: optionArray;
};
const processOptionArray = (optionArray, sourceData) => {
if (!Array.isArray(sourceData) || !sourceData.length || !sourceData[0]) return [];
const allEqual = optionArray.every((item, _, arr) => item === arr[0]);
const keyExistsInSource = Object.hasOwn(sourceData[0], String(optionArray[0]));
return allEqual && keyExistsInSource
? sourceData.map((d) => d[optionArray[0]])
: optionArray;
};


/**
* SourceData:
* [{
* "name": "Blue",
* "code": "name"
* },{
* "name": "Green",
* "code": "name"
* },{
* "name": "Red",
* "code": "name"
* }]
* The `Label key` in UI can take following values:
* 1. Normal string, without any quotes. e.g `name`
* This can be assumed as a key in each item of sourceData. We search it in each item of sourceData.
* 2. Except this everything comes in `{{}}`. It can have 2 types of values:
* a. Expressions that evaluate to a normal string. e.g `{{(() => `name`)()}}`
* In this case evaluated value will be ['name', 'name', 'name'].
* i. This can be assumed as a key in each item of sourceData. Handled by `allLabelsEqual` check.
* b. Dynamic property accessed via `item` object. e.g `{{item.name}}`
* In this case evaluated value will be actual values form sourceData ['Red', 'Green', 'Blue'].
* Hence we can assume that this array is the labels array.
* */
if (typeof props.optionLabel === "string") {
labels = sourceData.map((d) => d[props.optionLabel]);
} else if (_.isArray(props.optionLabel)) {
labels = sourceData.map((d, i) => d[props.optionLabel[i]]);
labels = processOptionArray(props.optionLabel, sourceData);
}

if (typeof props.optionValue === "string") {
values = sourceData.map((d) => d[props.optionValue]);
} else if (_.isArray(props.optionValue)) {
values = sourceData.map((d, i) => d[props.optionValue[i]]);
values = processOptionArray(props.optionValue, sourceData);
}

return sourceData.map((d, i) => ({
Expand Down
42 changes: 42 additions & 0 deletions app/client/src/widgets/SelectWidget/widget/propertyUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,27 @@ export function labelKeyValidation(
messages: [],
};
} else if (_.isArray(value)) {
/*
* Here assumption is that if evaluated array is all equal, then it is a key,
* and we can return the parsed value(from source data) as the options.
*/
const areAllValuesEqual = value.every((item, _, arr) => item === arr[0]);

if (
areAllValuesEqual &&
props.sourceData[0].hasOwnProperty(String(value[0]))
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Replace hasOwnProperty with Object.hasOwn

Direct hasOwnProperty calls are unsafe.

Use Object.hasOwn() instead of the prototype method call.

Also applies to: 250-250

🧰 Tools
🪛 Biome (1.9.4)

[error] 149-149: Do not access Object.prototype method 'hasOwnProperty' from target object.

It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.

(lint/suspicious/noPrototypeBuiltins)

) {
const parsedValue = props.sourceData.map(
(d: Record<string, unknown>) => d[String(value[0])],
);

return {
parsed: parsedValue,
isValid: true,
messages: [],
};
}
Comment on lines +141 to +160
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Refactor validation logic for better safety and reusability

The validation logic needs improvement in error handling and property access.

Consider extracting the common logic and adding safety checks:

+ const validateSourceDataKey = (value: unknown[], sourceData: unknown[]) => {
+   if (!Array.isArray(sourceData) || !sourceData.length || !sourceData[0]) {
+     return { isValid: false, parsedValue: [] };
+   }
+   
+   const areAllValuesEqual = value.every((item, _, arr) => item === arr[0]);
+   const firstValue = String(value[0]);
+   
+   if (areAllValuesEqual && Object.hasOwn(sourceData[0], firstValue)) {
+     return {
+       isValid: true,
+       parsedValue: sourceData.map((d: Record<string, unknown>) => d[firstValue]),
+     };
+   }
+   
+   return { isValid: false, parsedValue: [] };
+ };

Then use this helper in both validation functions.

Committable suggestion skipped: line range outside the PR's diff.

🧰 Tools
🪛 Biome (1.9.4)

[error] 149-149: Do not access Object.prototype method 'hasOwnProperty' from target object.

It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.

(lint/suspicious/noPrototypeBuiltins)


const errorIndex = value.findIndex((d) => !_.isString(d));

return {
Expand Down Expand Up @@ -218,6 +239,27 @@ export function valueKeyValidation(

options = sourceData.map((d: Record<string, unknown>) => d[value]);
} else if (_.isArray(value)) {
/*
* Here assumption is that if evaluated array is all equal, then it is a key,
* and we can return the parsed value(from source data) as the options.
*/
const areAllValuesEqual = value.every((item, _, arr) => item === arr[0]);

if (
areAllValuesEqual &&
props.sourceData[0].hasOwnProperty(String(value[0]))
) {
const parsedValue = props.sourceData.map(
(d: Record<string, unknown>) => d[String(value[0])],
);

return {
parsed: parsedValue,
isValid: true,
messages: [],
};
}
Comment on lines +242 to +261
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Remove duplicated validation logic

This block duplicates the logic from labelKeyValidation.

Use the suggested validateSourceDataKey helper function here as well.

🧰 Tools
🪛 Biome (1.9.4)

[error] 250-250: Do not access Object.prototype method 'hasOwnProperty' from target object.

It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.

(lint/suspicious/noPrototypeBuiltins)


const errorIndex = value.findIndex(
(d) =>
!(_.isString(d) || (_.isNumber(d) && !_.isNaN(d)) || _.isBoolean(d)),
Expand Down
Loading