Page MenuHomePhabricator
Differential D8787

[services-lib] Make DB attribute names non-static
ClosedPublic
Actions

Authored by bartek on Aug 10 2023, 3:33 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Apr 11, 8:26 AM
Unknown Object (File)
Fri, Mar 28, 8:03 AM
Unknown Object (File)
Wed, Mar 26, 11:52 AM
Unknown Object (File)
Fri, Mar 21, 12:31 AM
Unknown Object (File)
Mon, Mar 17, 12:07 AM
Unknown Object (File)
Mar 4 2025, 12:49 PM
Unknown Object (File)
Mar 4 2025, 12:49 PM
Unknown Object (File)
Mar 4 2025, 12:49 PM
View All Files
Subscribers
ashoat
tomek

Details

Reviewers
michal
• jon
varun
Commits
rCOMMff30037d57c5: [services-lib] Make DB attribute names non-static
Summary

Similiar to D8627 but in comm-services-lib. The difference is that I used impl Into<String> to add some flexibility

Test Plan

Following bash one-liner:

cd services
for r in "comm-services-lib" "backup" "blob" "feature-flags"; do pushd $r; cargo check --all-features; popd; done

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Revision Contents
Changeset List

PathSize
services/
blob/
src/
database/
2 lines
comm-services-lib/
src/
49 lines

Diff 29958

View Options

services/blob/src/database/types.rs

  • This file was added.
// @flow
import invariant from 'invariant';
import Alert from 'react-native/Libraries/Alert/Alert';
import {
sendReactionMessage,
sendReactionMessageActionTypes,
} from 'lib/actions/message-actions';
import type { BindServerCall, DispatchFunctions } from 'lib/utils/action-utils';
import type { TooltipRoute } from '../navigation/tooltip.react';
function onPressReact(
route:
| TooltipRoute<'TextMessageTooltipModal'>
| TooltipRoute<'MultimediaMessageTooltipModal'>,
dispatchFunctions: DispatchFunctions,
bindServerCall: BindServerCall,
) {
const messageID = route.params.item.messageInfo.id;
const threadID = route.params.item.threadInfo.id;
tomekUnsubmitted
Not Done
Inline Actions

This approach seems to be overly complicated. Can't we just write a function that receives messageID, threadID and reaction? This function can be named after an action that is performed, so react or sendReaction.

tomek: This approach seems to be overly complicated. Can't we just write a function that receives…
invariant(messageID, 'messageID should be set');
invariant(threadID, 'threadID should be set');
const callSendReactionMessage = bindServerCall(sendReactionMessage);
const reactionMessagePromise = (async () => {
try {
const result = await callSendReactionMessage(threadID, messageID, '👍');
return {
targetMessageID: messageID,
serverID: result.id,
threadID,
time: result.newMessageInfo.time,
interface: result.interface,
newMessageInfos: [result.newMessageInfo],
};
} catch (e) {
Alert.alert(
tomekUnsubmitted
Not Done
Inline Actions

It sounds risky to show one alert after every failed message. A user might react to a lot of messages in short period and a network issue could result in a lot of alerts being shown. Other apps sometimes choose to silently fail when a reaction fails.

tomek: It sounds risky to show one alert after every failed message. A user might react to a lot of…
ginsuAuthorUnsubmitted
Done
Inline Actions

Totally see your concern here! I spoke to @atul about this today, and we both agreed that we want this to be a temporary solution, but since reaction messages don't have retry behavior we think it is best to show some sort of feedback to the user in case something failed.

ginsu: Totally see your concern here! I spoke to @atul about this today, and we both agreed that we…
'Couldn’t send the reaction',
'Please try again later',
[{ text: 'OK' }],
{
cancelable: false,
tomekUnsubmitted
Not Done
Inline Actions

Is there a reason to set cancelable to false?

tomek: Is there a reason to set cancelable to false?
ginsuAuthorUnsubmitted
Done
Inline Actions

Alerts on Android can be dismissed by tapping outside of the alert box. It is disabled by default and can be enabled by providing an optional Options parameter with the cancelable property set to true. Here is more context

I also found the alert on report-utils to follow the same patern

ginsu: Alerts on Android can be dismissed by tapping outside of the alert box. It is disabled by…
},
);
throw e;
}
})();
dispatchFunctions.dispatchActionPromise(
sendReactionMessageActionTypes,
reactionMessagePromise,
);
}
export { onPressReact };
View Options

services/comm-services-lib/src/database.rs

  • This file was added.
// @flow
import invariant from 'invariant';
import Alert from 'react-native/Libraries/Alert/Alert';
import {
sendReactionMessage,
sendReactionMessageActionTypes,
} from 'lib/actions/message-actions';
import type { BindServerCall, DispatchFunctions } from 'lib/utils/action-utils';
import type { TooltipRoute } from '../navigation/tooltip.react';
function onPressReact(
route:
| TooltipRoute<'TextMessageTooltipModal'>
| TooltipRoute<'MultimediaMessageTooltipModal'>,
dispatchFunctions: DispatchFunctions,
bindServerCall: BindServerCall,
) {
const messageID = route.params.item.messageInfo.id;
const threadID = route.params.item.threadInfo.id;
tomekUnsubmitted
Not Done
Inline Actions

This approach seems to be overly complicated. Can't we just write a function that receives messageID, threadID and reaction? This function can be named after an action that is performed, so react or sendReaction.

tomek: This approach seems to be overly complicated. Can't we just write a function that receives…
invariant(messageID, 'messageID should be set');
invariant(threadID, 'threadID should be set');
const callSendReactionMessage = bindServerCall(sendReactionMessage);
const reactionMessagePromise = (async () => {
try {
const result = await callSendReactionMessage(threadID, messageID, '👍');
return {
targetMessageID: messageID,
serverID: result.id,
threadID,
time: result.newMessageInfo.time,
interface: result.interface,
newMessageInfos: [result.newMessageInfo],
};
} catch (e) {
Alert.alert(
tomekUnsubmitted
Not Done
Inline Actions

It sounds risky to show one alert after every failed message. A user might react to a lot of messages in short period and a network issue could result in a lot of alerts being shown. Other apps sometimes choose to silently fail when a reaction fails.

tomek: It sounds risky to show one alert after every failed message. A user might react to a lot of…
ginsuAuthorUnsubmitted
Done
Inline Actions

Totally see your concern here! I spoke to @atul about this today, and we both agreed that we want this to be a temporary solution, but since reaction messages don't have retry behavior we think it is best to show some sort of feedback to the user in case something failed.

ginsu: Totally see your concern here! I spoke to @atul about this today, and we both agreed that we…
'Couldn’t send the reaction',
'Please try again later',
[{ text: 'OK' }],
{
cancelable: false,
tomekUnsubmitted
Not Done
Inline Actions

Is there a reason to set cancelable to false?

tomek: Is there a reason to set cancelable to false?
ginsuAuthorUnsubmitted
Done
Inline Actions

Alerts on Android can be dismissed by tapping outside of the alert box. It is disabled by default and can be enabled by providing an optional Options parameter with the cancelable property set to true. Here is more context

I also found the alert on report-utils to follow the same patern

ginsu: Alerts on Android can be dismissed by tapping outside of the alert box. It is disabled by…
},
);
throw e;
}
})();
dispatchFunctions.dispatchActionPromise(
sendReactionMessageActionTypes,
reactionMessagePromise,
);
}
export { onPressReact };