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

Subscribe change to subscribev2 #1524

Merged
merged 4 commits into from
May 28, 2024

Conversation

abhishek-01k
Copy link
Collaborator

Changed the subscribe API to subscribeV2 for Opt In and now the user settings are getting updated as per the API call.

Copy link

vercel bot commented May 14, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
push-dapp ❌ Failed (Inspect) May 27, 2024 11:02am

Copy link

In OptinNotifSettingDropdownContainer component:

  1. In the handleSwitchChange function, there is a reference to updatedSettings, but it should be modifiedSettings. Change updatedSettings[index].type to modifiedSettings[index].type.
  2. The closing curly braces for the component and the file are missing. Add }; at the end of the component and } at the end of the file.

In OptinNotifSettingDropdown component:

  1. You need to add closing curly braces for the toggleDropdown and closeDropdown functions.
  2. There is a missing } after the closing (wallet?.accounts?.length > 0)) {.
  3. Add missing } at the end of the component before the export statement.

In notifChannelSettingFormatString function of notifSetting.ts:

  1. There is a missing closing curly brace } for the function.

In notifUserSettingFormatString function of notifSetting.ts:

  1. There is an incomplete ternary operator. Update ? _notifSettings.push({ value: setting.user, enabled: (setting as ChannelSetting & { type: 2 }).enabled }) to : _notifSettings.push({ value: setting.user, enabled: (setting as ChannelSetting & { type: 2 }).enabled }).

In userSettingsFromDefaultChannelSetting function of notifSetting.ts:

  1. There are incomplete ternary operators and missing closing curly braces. Update ? _userSettings.push({ ...setting, user: setting.default }) to : _userSettings.push({ ...setting, user: setting.default }).

Overall, the code seems to have several issues related to missing closing curly braces and incorrect references. Make the mentioned corrections in the respective files.

Copy link
Collaborator

@rohitmalhotra1420 rohitmalhotra1420 left a comment

Choose a reason for hiding this comment

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

@corlard3y could you also give it a functional review once @abhishek-01k is done with the changes.

@abhishek-01k a loom video with a proper explanation about the changes is much appreciated on this PR.

Comment on lines 54 to 86
return null;
}

let userSetting = '';
let numberOfSettings = 0;
for (let i = 0; i < settings.length; i++) {
const ele = settings[i];
const enabled = ele.enabled ? 1 : 0;
if (ele.enabled) numberOfSettings++;

if (Object.keys(ele).includes('value')) {
// slider type
if (typeof ele.value == 'number')
userSetting = userSetting + SLIDER_TYPE + SETTING_DELIMITER + enabled + SETTING_DELIMITER + ele.value;
else {
userSetting =
userSetting +
RANGE_TYPE +
SETTING_DELIMITER +
enabled +
SETTING_DELIMITER +
ele.value?.lower +
SETTING_DELIMITER +
ele.value?.upper;
}
} else {
// boolean type
userSetting = userSetting + BOOLEAN_TYPE + SETTING_DELIMITER + enabled;
}
if (i != settings.length - 1) userSetting = userSetting + SETTING_SEPARATOR;
}
return numberOfSettings + SETTING_SEPARATOR + userSetting;
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • Let's break down this function into more smaller utility functions and re-use them here.
  • Replace the for loop with array.reduce( preffered) or array.forEach (less preffered).

src/helpers/channel/notifSetting.ts Show resolved Hide resolved
src/helpers/channel/notifSetting.ts Outdated Show resolved Hide resolved
@mishramonalisha76 mishramonalisha76 requested review from mishramonalisha76 and removed request for 0xNilesh May 14, 2024 13:40
Copy link
Contributor

@mishramonalisha76 mishramonalisha76 left a comment

Choose a reason for hiding this comment

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

Functionality looks good to me

Copy link

In the file OptinNotifSettingDropdown.tsx:

  1. In the OptinNotifSettingDropdownContainer component, there seems to be a missing closing brace '}' at the end of the component.
  2. In the OptinNotifSettingDropdown component, there are missing closing braces '}' for the toggleDropdown and closeDropdown functions.
  3. The handleSwitchChange function is referencing 'updatedSettings' which is not declared inside the function. It should reference 'modifiedSettings' instead.
  4. There is a missing closing brace '}' at the end of the useState hook for web3Provider and walletAddress assignment.
  5. In the optInHandler function, there is a missing closing brace '}' at the end of the function.
  6. There is a missing closing brace '}' at the end of the toggleDropdown function.
  7. The subscribeToast function is missing a closing brace '}' at the end.
  8. There is a missing closing brace '}' at the end of the channelAddress assignment block inside the optInHandler function.
  9. The onError callback in the PushAPI.channels.subscribeV2 is incorrectly defined. It should be a function.
  10. The commented out code block for PUSHAPI.channels.subscribe is not properly commented.

In the file InputSlider.tsx:

  1. The Inactive styled component is missing a closing brace '}'.
  2. The handleMouseMove function is missing a closing brace '}'.
  3. The handleMouseUp function is missing a closing brace '}'.
  4. The showPreview function is missing a closing brace '}'.
  5. The hidePreview function is missing a closing brace '}'.

Overall, the missing closing braces and incorrect references should be fixed.

In the file RangeSlider.tsx:

No content provided.

File: src/helpers/channel/notifSetting.ts:

  1. The notifChannelSettingFormatString function is missing a closing brace '}' at the end of the function.
  2. The notifUserSettingFormatString function has a code block that is commented but not properly commented out.
  3. The userSettingsFromDefaultChannelSetting function is missing a closing brace '}' at the end of the function.
  4. There are several constants declared but not used. It's recommended to review their necessity.

After addressing the above issues, review the remaining logic to ensure all functionalities are correct.

" All looks good. "

Copy link

github-actions bot commented May 24, 2024

PR Preview Action v1.4.7
Preview removed because the pull request was closed.
2024-05-28 13:15 UTC

Copy link

In the file 'src/components/dropdowns/OptinNotifSettingDropdown.tsx':

  1. In the OptinNotifSettingDropdownContainer component, there is a missing closing curly brace '}' after the interface block.

  2. In the OptinNotifSettingDropdown component, the closeDropdown function is missing a closing curly brace '}'

  3. There are syntax errors in the optInHandler function. The 'try' block is not closed properly after the 'else' block.

In the file 'src/components/reusables/sliders/InputSlider.tsx':

  1. In the styled-components definition for Inactive, the closing backtick is missing.

  2. In the Container styled-component, there should be a closing curly brace '}' at the end of the definition.

  3. The showPreview and hidePreview functions are missing closing curly braces '}'.

In the file 'src/helpers/channel/notifSetting.ts':

  1. In the notifChannelSettingFormatString function, there is a syntax error in the final return statement. The closing curly brace '}' is missing for the function.

Once these issues are addressed, you can re-run the code and test it.

Copy link
Collaborator

@rohitmalhotra1420 rohitmalhotra1420 left a comment

Choose a reason for hiding this comment

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

Non functional minor fixes, but I am approving it.

Next steps:

  • Get this tested from Kartikey
  • Ask him to post his QA finding on the ticket.
  • If Qa is successful merge the PR and ask @rohitmalhotra1420 to close this ticket.

src/helpers/channel/notifSetting.ts Outdated Show resolved Hide resolved
src/helpers/channel/notifSetting.ts Outdated Show resolved Hide resolved
Copy link

In the file src/components/dropdowns/OptinNotifSettingDropdown.tsx:

  1. The closing curly brace for the OptinNotifSettingDropdownContainerProps interface is missing.
  2. The updatedSettings variable is used in handleSwitchChange function but it is not defined within that function.
  3. There are missing closing curly braces for the toggleDropdown and closeDropdown functions.
  4. The subscribeToast.showLoaderToast function call is missing a closing parenthesis.
  5. The onDragStart, onDragEnd, showPreview, and hidePreview functions are missing closing curly braces.

Overall, the code seems to have several syntax errors and missing closing braces.

In the file src/components/reusables/sliders/InputSlider.tsx:

  1. The Inactive styled component is missing a closing curly brace.
  2. The handleMouseMove function is missing a closing brace before the onChange call.
  3. The handleMouseUp function is missing an opening brace.
  4. The showPreview and hidePreview functions are missing closing braces.

There are several syntax errors and missing braces in this file as well.

In the file src/helpers/channel/notifSetting.ts:

  1. There are missing closing braces for several condition blocks.
  2. There is an incomplete conditional statement in the notifChannelSettingFormatString function.
  3. In the notifUserSettingFormatString function, there is a conditional statement with an incomplete true branch.
  4. The userSettingsFromDefaultChannelSetting function has incomplete conditional statements within its forEach loop.

There are several incomplete conditional statements and missing braces in this file.

Kindly revise these issues and ensure proper syntax and brace matching for the code to be error-free.

All looks good.

@GeekyKartikey
Copy link

I have done the QA for this, the opt-in feature is working and also the notification settings are working without any issues. It seems well to me

@rohitmalhotra1420 rohitmalhotra1420 merged commit 75c87a5 into main May 28, 2024
3 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🐛 [BUG] - Change the notifications.subscribe to subscribeV2 to add settings for the users
4 participants