Skip to content

[Refactor] SetNameValuePair #9677

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
marcaaron merged 14 commits into from
Jul 19, 2022
Merged

[Refactor] SetNameValuePair #9677

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
8679228
initial development
danieldoglas Jul 1, 2022
50840cf
refactoring and fixing requests
danieldoglas Jul 1, 2022
9c5a656
refactor methods to use named methods
danieldoglas Jul 4, 2022
471a8ea
fixing styles and new methods
danieldoglas Jul 5, 2022
6977763
reverting changes for first time usage
danieldoglas Jul 6, 2022
2b7af34
Merge branch 'main' into dsilva_refactorSetNameValuePairs
danieldoglas Jul 12, 2022
20db056
fixing style and bug
danieldoglas Jul 12, 2022
883f74c
changing string to constant
danieldoglas Jul 12, 2022
03f22e1
changing string to constant
danieldoglas Jul 12, 2022
f4a669b
merge
danieldoglas Jul 12, 2022
d2ad164
renaming methods to right naming convention
danieldoglas Jul 13, 2022
08b420d
solving issue on merge for comment
danieldoglas Jul 13, 2022
90ddc6b
merge main
danieldoglas Jul 18, 2022
dcd38a4
solving merge issue
danieldoglas Jul 18, 2022
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
1 change: 1 addition & 0 deletions src/CONST.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -734,6 +734,7 @@ const CONST = {
ONYX: {
METHOD: {
MERGE: 'merge',
SET: 'set',
},
},
};
Expand Down
2 changes: 1 addition & 1 deletion src/components/EmojiPicker/EmojiPickerMenu/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -389,7 +389,7 @@ class EmojiPickerMenu extends Component {
return;
}

User.setPreferredSkinTone(skinTone);
User.updatePreferredSkinTone(skinTone);
}

/**
Expand Down
2 changes: 1 addition & 1 deletion src/components/EmojiPicker/EmojiPickerMenu/index.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ class EmojiPickerMenu extends Component {
return;
}

User.setPreferredSkinTone(skinTone);
User.updatePreferredSkinTone(skinTone);
}

/**
Expand Down
3 changes: 1 addition & 2 deletions src/libs/EmojiUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,8 +196,7 @@ function addToFrequentlyUsedEmojis(frequentlyUsedEmojis, newEmoji) {

// Second sorting is required so that new emoji is properly placed at sort-ordered location
frequentEmojiList = lodashOrderBy(frequentEmojiList, ['count', 'lastUpdatedAt'], ['desc', 'desc']);

User.setFrequentlyUsedEmojis(frequentEmojiList);
User.updateFrequentlyUsedEmojis(frequentEmojiList);
}

export {
Expand Down
5 changes: 2 additions & 3 deletions src/libs/actions/PaymentMethods.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import * as Localize from '../Localize';
import Navigation from '../Navigation/Navigation';
import * as CardUtils from '../CardUtils';
import ROUTES from '../../ROUTES';
import NameValuePair from './NameValuePair';
import * as User from './User';
import * as store from './ReimbursementAccount/store';

/**
Expand All @@ -37,8 +37,7 @@ function deleteDebitCard(fundID) {
}

function deletePayPalMe() {
NameValuePair.set(CONST.NVP.PAYPAL_ME_ADDRESS, '');
Onyx.set(ONYXKEYS.NVP_PAYPAL_ME_ADDRESS, null);
User.deletePaypalMeAddress();
Growl.show(Localize.translateLocal('paymentsPage.deletePayPalSuccess'), CONST.GROWL.SUCCESS, 3000);
}

Expand Down
85 changes: 78 additions & 7 deletions src/libs/actions/User.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ import * as Pusher from '../Pusher/pusher';
import Log from '../Log';
import NetworkConnection from '../NetworkConnection';
import redirectToSignIn from './SignInRedirect';
import NameValuePair from './NameValuePair';
import Growl from '../Growl';
import * as Localize from '../Localize';
import * as CloseAccountActions from './CloseAccount';
Expand Down Expand Up @@ -267,6 +266,40 @@ function isBlockedFromConcierge(blockedFromConcierge) {
return moment().isBefore(moment(blockedFromConcierge.expiresAt), 'day');
}

/**
* Adds a paypal.me address for the user
*
* @param {String} address
*/
function addPaypalMeAddress(address) {
const optimisticData = [
{
onyxMethod: CONST.ONYX.METHOD.MERGE,
key: ONYXKEYS.NVP_PAYPAL_ME_ADDRESS,
value: address,
},
];
API.write('AddPaypalMeAddress', {
value: address,
}, {optimisticData});
}

@marcaaron marcaaron Jul 19, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm actually curious why we are double storing the paypalme address... ?

App/src/libs/actions/PersonalDetails.js

Lines 100 to 111 in 13d2766

const payPalMeAddress = details.expensify_payPalMeAddress || '';
const phoneNumber = details.phoneNumber || '';
const avatarHighResolution = details.avatar || details.avatarThumbnail;
formattedResult[sanitizedLogin] = {
login: sanitizedLogin,
avatar,
displayName,
firstName,
lastName,
pronouns,
timezone,
payPalMeAddress,

I think maybe we would just refer to personalDetails[currentUserEmail].payPalMeAddress everywhere. I don't have the full context on why we are storing this in two places.

@marcaaron marcaaron Jul 19, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe we can combine them (also maybe makes sense to do this in a follow-up though - not sure)

@danieldoglas danieldoglas Jul 19, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I also agree that this should be a follow up. Considering we have Paypal as part of the payment methods, we shouldn't be calling "UpdatePersonalDetails" from that screen. we can unify it, but we need to decide where it's going to be the better place.


/**
* Deletes a paypal.me address for the user
*
*/
function deletePaypalMeAddress() {
const optimisticData = [
{
onyxMethod: CONST.ONYX.METHOD.MERGE,
key: ONYXKEYS.NVP_PAYPAL_ME_ADDRESS,
value: '',
},
];
API.write('DeletePaypalMeAddress', {}, {optimisticData});

@sketchydroide sketchydroide Jul 13, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

even if there is no value it feels weird to have this in a single line, unless linter asks it to be like this I would have in multiple lines

@danieldoglas danieldoglas Jul 13, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

At first it was on multiple lines and it got strange too. What's your suggestion?

API.write('DeletePaypalMeAddress', 
{}, {optimisticData});

?

Growl.show(Localize.translateLocal('paymentsPage.deletePayPalSuccess'), CONST.GROWL.SUCCESS, 3000);
}

/**
* Initialize our pusher subscription to listen for user changes
*/
Expand Down Expand Up @@ -351,16 +384,51 @@ function subscribeToExpensifyCardUpdates() {
* Sync preferredSkinTone with Onyx and Server
* @param {String} skinTone
*/
function setPreferredSkinTone(skinTone) {
NameValuePair.set(CONST.NVP.PREFERRED_EMOJI_SKIN_TONE, skinTone, ONYXKEYS.PREFERRED_EMOJI_SKIN_TONE);
function updatePreferredSkinTone(skinTone) {
const optimisticData = [
{
onyxMethod: CONST.ONYX.METHOD.SET,
key: ONYXKEYS.PREFERRED_EMOJI_SKIN_TONE,
value: skinTone,
},
];
API.write('UpdatePreferredEmojiSkinTone', {
value: skinTone,
}, {optimisticData});
}

/**
* Sync frequentlyUsedEmojis with Onyx and Server
* @param {Object[]} frequentlyUsedEmojis
*/
function setFrequentlyUsedEmojis(frequentlyUsedEmojis) {
NameValuePair.set(CONST.NVP.FREQUENTLY_USED_EMOJIS, frequentlyUsedEmojis, ONYXKEYS.FREQUENTLY_USED_EMOJIS);
function updateFrequentlyUsedEmojis(frequentlyUsedEmojis) {
const optimisticData = [
{
onyxMethod: CONST.ONYX.METHOD.SET,
key: ONYXKEYS.FREQUENTLY_USED_EMOJIS,
value: frequentlyUsedEmojis,
},
];
API.write('UpdateFrequentlyUsedEmojis', {
value: JSON.stringify(frequentlyUsedEmojis),
}, {optimisticData});
}

/**
* Sync user chat priority mode with Onyx and Server
* @param {String} mode
*/
function updateChatPriorityMode(mode) {
const optimisticData = [
{
onyxMethod: CONST.ONYX.METHOD.MERGE,
key: ONYXKEYS.NVP_PRIORITY_MODE,
value: mode,
},
];
API.write('UpdateChatPriorityMode', {
value: mode,
}, {optimisticData});
}

/**
Expand Down Expand Up @@ -422,12 +490,15 @@ export {
validateLogin,
isBlockedFromConcierge,
subscribeToUserEvents,
setPreferredSkinTone,
updatePreferredSkinTone,
setShouldUseSecureStaging,
clearUserErrorMessage,
subscribeToExpensifyCardUpdates,
setFrequentlyUsedEmojis,
updateFrequentlyUsedEmojis,
joinScreenShare,
clearScreenShareRequest,
generateStatementPDF,
deletePaypalMeAddress,
addPaypalMeAddress,
updateChatPriorityMode,
};
5 changes: 3 additions & 2 deletions src/pages/settings/Payments/AddPayPalMePage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import ROUTES from '../../../ROUTES';
import HeaderWithCloseButton from '../../../components/HeaderWithCloseButton';
import Text from '../../../components/Text';
import ScreenWrapper from '../../../components/ScreenWrapper';
import NameValuePair from '../../../libs/actions/NameValuePair';
import Navigation from '../../../libs/Navigation/Navigation';
import styles from '../../../styles/styles';
import withLocalize, {withLocalizePropTypes} from '../../../components/withLocalize';
Expand All @@ -19,6 +18,7 @@ import FixedFooter from '../../../components/FixedFooter';
import Growl from '../../../libs/Growl';
import TextInput from '../../../components/TextInput';
import * as ValidationUtils from '../../../libs/ValidationUtils';
import * as User from '../../../libs/actions/User';

const propTypes = {
/** Username for PayPal.Me */
Expand Down Expand Up @@ -52,7 +52,8 @@ class AddPayPalMePage extends React.Component {
return;
}
this.setState({payPalMeUsernameError: false});
NameValuePair.set(CONST.NVP.PAYPAL_ME_ADDRESS, this.state.payPalMeUsername, ONYXKEYS.NVP_PAYPAL_ME_ADDRESS);
User.addPaypalMeAddress(this.state.payPalMeUsername);

Growl.show(this.props.translate('addPayPalMePage.growlMessageOnSave'), CONST.GROWL.SUCCESS, 3000);
Navigation.navigate(ROUTES.SETTINGS_PAYMENTS);
}
Expand Down
3 changes: 1 addition & 2 deletions src/pages/settings/PreferencesPage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import ROUTES from '../../ROUTES';
import ONYXKEYS from '../../ONYXKEYS';
import styles from '../../styles/styles';
import Text from '../../components/Text';
import NameValuePair from '../../libs/actions/NameValuePair';
import CONST from '../../CONST';
import * as User from '../../libs/actions/User';
import ScreenWrapper from '../../components/ScreenWrapper';
Expand Down Expand Up @@ -87,7 +86,7 @@ const PreferencesPage = (props) => {
<Picker
label={props.translate('preferencesPage.priorityMode')}
onInputChange={
mode => NameValuePair.set(CONST.NVP.PRIORITY_MODE, mode, ONYXKEYS.NVP_PRIORITY_MODE)
mode => User.updateChatPriorityMode(mode)
}
items={_.values(priorityModes)}
value={props.priorityMode}
Expand Down