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

Add comment text update by edit comment #51422

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
34 changes: 23 additions & 11 deletions src/libs/Network/SequentialQueue.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import * as QueuedOnyxUpdates from '@userActions/QueuedOnyxUpdates';
import CONST from '@src/CONST';
import ONYXKEYS from '@src/ONYXKEYS';
import type OnyxRequest from '@src/types/onyx/Request';
import type {ConflictData} from '@src/types/onyx/Request';
import * as NetworkStore from './NetworkStore';

type RequestError = Error & {
Expand Down Expand Up @@ -96,15 +97,15 @@ function process(): Promise<void> {
pause();
}

PersistedRequests.remove(requestToProcess);
PersistedRequests.endRequestAndRemoveFromQueue(requestToProcess);
RequestThrottle.clear();
return process();
})
.catch((error: RequestError) => {
// On sign out we cancel any in flight requests from the user. Since that user is no longer signed in their requests should not be retried.
// Duplicate records don't need to be retried as they just mean the record already exists on the server
if (error.name === CONST.ERROR.REQUEST_CANCELLED || error.message === CONST.ERROR.DUPLICATE_RECORD) {
PersistedRequests.remove(requestToProcess);
PersistedRequests.endRequestAndRemoveFromQueue(requestToProcess);
RequestThrottle.clear();
return process();
}
Expand All @@ -113,7 +114,7 @@ function process(): Promise<void> {
.then(process)
.catch(() => {
Onyx.update(requestToProcess.failureData ?? []);
PersistedRequests.remove(requestToProcess);
PersistedRequests.endRequestAndRemoveFromQueue(requestToProcess);
RequestThrottle.clear();
return process();
});
Expand Down Expand Up @@ -204,6 +205,24 @@ function isPaused(): boolean {
// Flush the queue when the connection resumes
NetworkStore.onReconnection(flush);

function handleConflictActions(conflictAction: ConflictData, newRequest: OnyxRequest) {
if (conflictAction.type === 'push') {
PersistedRequests.save(newRequest);
} else if (conflictAction.type === 'replace') {
PersistedRequests.update(conflictAction.index, conflictAction.request ?? newRequest);
} else if (conflictAction.type === 'delete') {
PersistedRequests.deleteRequestsByIndices(conflictAction.indices);
if (conflictAction.pushNewRequest) {
PersistedRequests.save(newRequest);
}
if (conflictAction.nextAction) {
handleConflictActions(conflictAction.nextAction, newRequest);
}
} else {
Log.info(`[SequentialQueue] No action performed to command ${newRequest.command} and it will be ignored.`);
}
}

function push(newRequest: OnyxRequest) {
const {checkAndFixConflictingRequest} = newRequest;

Expand All @@ -215,14 +234,7 @@ function push(newRequest: OnyxRequest) {
// don't try to serialize a function.
// eslint-disable-next-line no-param-reassign
delete newRequest.checkAndFixConflictingRequest;

if (conflictAction.type === 'push') {
PersistedRequests.save(newRequest);
} else if (conflictAction.type === 'replace') {
PersistedRequests.update(conflictAction.index, newRequest);
} else {
Log.info(`[SequentialQueue] No action performed to command ${newRequest.command} and it will be ignored.`);
}
handleConflictActions(conflictAction, newRequest);
} else {
// Add request to Persisted Requests so that it can be retried if it fails
PersistedRequests.save(newRequest);
Expand Down
19 changes: 16 additions & 3 deletions src/libs/actions/PersistedRequests.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ function save(requestToPersist: Request) {
});
}

function remove(requestToRemove: Request) {
function endRequestAndRemoveFromQueue(requestToRemove: Request) {
ongoingRequest = null;
/**
* We only remove the first matching request because the order of requests matters.
Expand All @@ -76,6 +76,19 @@ function remove(requestToRemove: Request) {
});
}

function deleteRequestsByIndices(indices: number[]) {
// Create a Set from the indices array for efficient lookup
const indicesSet = new Set(indices);

// Create a new array excluding elements at the specified indices
persistedRequests = persistedRequests.filter((_, index) => !indicesSet.has(index));

// Update the persisted requests in storage or state as necessary
Onyx.set(ONYXKEYS.PERSISTED_REQUESTS, persistedRequests).then(() => {
Log.info(`Multiple (${indices.length}) requests removed from the queue. Queue length is ${persistedRequests.length}`);
});
}

function update(oldRequestIndex: number, newRequest: Request) {
const requests = [...persistedRequests];
requests.splice(oldRequestIndex, 1, newRequest);
Expand Down Expand Up @@ -117,7 +130,7 @@ function rollbackOngoingRequest() {
}

// Prepend ongoingRequest to persistedRequests
persistedRequests.unshift(ongoingRequest);
persistedRequests.unshift({...ongoingRequest, isRollbacked: true});

// Clear the ongoingRequest
ongoingRequest = null;
Expand All @@ -131,4 +144,4 @@ function getOngoingRequest(): Request | null {
return ongoingRequest;
}

export {clear, save, getAll, remove, update, getLength, getOngoingRequest, processNextRequest, updateOngoingRequest, rollbackOngoingRequest};
export {clear, save, getAll, endRequestAndRemoveFromQueue, update, getLength, getOngoingRequest, processNextRequest, updateOngoingRequest, rollbackOngoingRequest, deleteRequestsByIndices};
21 changes: 17 additions & 4 deletions src/libs/actions/Report.ts
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ import {isEmptyObject} from '@src/types/utils/EmptyObject';
import * as CachedPDFPaths from './CachedPDFPaths';
import * as Modal from './Modal';
import navigateFromNotification from './navigateFromNotification';
import {createUpdateCommentMatcher, resolveDuplicationConflictAction} from './RequestConflictUtils';
import {createUpdateCommentMatcher, resolveCommentDeletionConflicts, resolveDuplicationConflictAction, resolveEditCommentWithNewAddCommentRequest} from './RequestConflictUtils';
import * as Session from './Session';
import * as Welcome from './Welcome';
import * as OnboardingFlow from './Welcome/OnboardingFlow';
Expand Down Expand Up @@ -162,7 +162,7 @@ type GuidedSetupData = Array<
type ReportError = {
type?: string;
};

const addNewMessageWithText = new Set<string>([WRITE_COMMANDS.ADD_COMMENT, WRITE_COMMANDS.ADD_TEXT_AND_ATTACHMENT]);
let conciergeChatReportID: string | undefined;
let currentUserAccountID = -1;
let currentUserEmail: string | undefined;
Expand Down Expand Up @@ -1535,7 +1535,14 @@ function deleteReportComment(reportID: string, reportAction: ReportAction) {

CachedPDFPaths.clearByKey(reportActionID);

API.write(WRITE_COMMANDS.DELETE_COMMENT, parameters, {optimisticData, successData, failureData});
API.write(
WRITE_COMMANDS.DELETE_COMMENT,
parameters,
{optimisticData, successData, failureData},
{
checkAndFixConflictingRequest: (persistedRequests) => resolveCommentDeletionConflicts(persistedRequests, reportActionID, originalReportID),
},
);

// if we are linking to the report action, and we are deleting it, and it's not a deleted parent action,
// we should navigate to its report in order to not show not found page
Expand Down Expand Up @@ -1700,7 +1707,13 @@ function editReportComment(reportID: string, originalReportAction: OnyxEntry<Rep
parameters,
{optimisticData, successData, failureData},
{
checkAndFixConflictingRequest: (persistedRequests) => resolveDuplicationConflictAction(persistedRequests, createUpdateCommentMatcher(reportActionID)),
checkAndFixConflictingRequest: (persistedRequests) => {
const addCommentIndex = persistedRequests.findIndex((request) => addNewMessageWithText.has(request.command) && request.data?.reportActionID === reportActionID);
if (addCommentIndex > -1) {
return resolveEditCommentWithNewAddCommentRequest(persistedRequests, parameters, reportActionID, addCommentIndex);
}
return resolveDuplicationConflictAction(persistedRequests, createUpdateCommentMatcher(reportActionID));
},
},
);
}
Expand Down
115 changes: 112 additions & 3 deletions src/libs/actions/RequestConflictUtils.ts
Original file line number Diff line number Diff line change
@@ -1,15 +1,30 @@
import type {OnyxUpdate} from 'react-native-onyx';
import Onyx from 'react-native-onyx';
import type {UpdateCommentParams} from '@libs/API/parameters';
import {WRITE_COMMANDS} from '@libs/API/types';
import ONYXKEYS from '@src/ONYXKEYS';
import type OnyxRequest from '@src/types/onyx/Request';
import type {ConflictActionData} from '@src/types/onyx/Request';

type RequestMatcher = (request: OnyxRequest) => boolean;

const addNewMessage = new Set<string>([WRITE_COMMANDS.ADD_COMMENT, WRITE_COMMANDS.ADD_ATTACHMENT, WRITE_COMMANDS.ADD_TEXT_AND_ATTACHMENT]);

const commentsToBeDeleted = new Set<string>([
WRITE_COMMANDS.ADD_COMMENT,
WRITE_COMMANDS.ADD_ATTACHMENT,
WRITE_COMMANDS.ADD_TEXT_AND_ATTACHMENT,
WRITE_COMMANDS.UPDATE_COMMENT,
WRITE_COMMANDS.ADD_EMOJI_REACTION,
WRITE_COMMANDS.REMOVE_EMOJI_REACTION,
]);

function createUpdateCommentMatcher(reportActionID: string) {
return function (request: OnyxRequest) {
return request.command === WRITE_COMMANDS.UPDATE_COMMENT && request.data?.reportActionID === reportActionID;
};
}

type RequestMatcher = (request: OnyxRequest) => boolean;

/**
* Determines the appropriate action for handling duplication conflicts in persisted requests.
*
Expand All @@ -35,4 +50,98 @@ function resolveDuplicationConflictAction(persistedRequests: OnyxRequest[], requ
};
}

export {resolveDuplicationConflictAction, createUpdateCommentMatcher};
function resolveCommentDeletionConflicts(persistedRequests: OnyxRequest[], reportActionID: string, originalReportID: string): ConflictActionData {
const commentIndicesToDelete: number[] = [];
const commentCouldBeThread: Record<string, number> = {};
let addCommentFound = false;
persistedRequests.forEach((request, index) => {
// If the request will open a Thread, we should not delete the comment and we should send all the requests
if (request.command === WRITE_COMMANDS.OPEN_REPORT && request.data?.parentReportActionID === reportActionID && reportActionID in commentCouldBeThread) {
const indexToRemove = commentCouldBeThread[reportActionID];
commentIndicesToDelete.splice(indexToRemove, 1);
// The new message performs some changes in Onyx, we want to keep those changes.
addCommentFound = false;
return;
}

if (!commentsToBeDeleted.has(request.command) || request.data?.reportActionID !== reportActionID) {
return;
}

// If we find a new message, we probably want to remove it and not perform any request given that the server
// doesn't know about it yet.
if (addNewMessage.has(request.command) && !request.isRollbacked) {
addCommentFound = true;
commentCouldBeThread[reportActionID] = commentIndicesToDelete.length;
}
commentIndicesToDelete.push(index);
});

if (commentIndicesToDelete.length === 0) {
return {
conflictAction: {
type: 'push',
},
};
}

if (addCommentFound) {
// The new message performs some changes in Onyx, so we need to rollback those changes.
const rollbackData: OnyxUpdate[] = [
{
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${originalReportID}`,
value: {
[reportActionID]: null,
},
},
];
Onyx.update(rollbackData);
}

return {
conflictAction: {
type: 'delete',
indices: commentIndicesToDelete,
pushNewRequest: !addCommentFound,
},
};
}

function resolveEditCommentWithNewAddCommentRequest(persistedRequests: OnyxRequest[], parameters: UpdateCommentParams, reportActionID: string, addCommentIndex: number): ConflictActionData {
const indicesToDelete: number[] = [];
persistedRequests.forEach((request, index) => {
if (request.command !== WRITE_COMMANDS.UPDATE_COMMENT || request.data?.reportActionID !== reportActionID) {
return;
}
indicesToDelete.push(index);
});

const currentAddComment = persistedRequests.at(addCommentIndex);
let nextAction = null;
if (currentAddComment) {
currentAddComment.data = {...currentAddComment.data, ...parameters};
nextAction = {
type: 'replace',
index: addCommentIndex,
request: currentAddComment,
};

if (indicesToDelete.length === 0) {
return {
conflictAction: nextAction,
} as ConflictActionData;
}
}

return {
conflictAction: {
type: 'delete',
indices: indicesToDelete,
pushNewRequest: false,
nextAction,
},
} as ConflictActionData;
}

export {resolveDuplicationConflictAction, resolveCommentDeletionConflicts, resolveEditCommentWithNewAddCommentRequest, createUpdateCommentMatcher};
44 changes: 42 additions & 2 deletions src/types/onyx/Request.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,11 @@ type RequestData = {
shouldSkipWebProxy?: boolean;
};

/**
* Represents the possible actions to take in case of a conflict in the request queue.
*/
type ConflictData = ConflictRequestReplace | ConflictRequestDelete | ConflictRequestPush | ConflictRequestNoAction;

/**
* Model of a conflict request that has to be replaced in the request queue.
*/
Expand All @@ -68,6 +73,36 @@ type ConflictRequestReplace = {
* The index of the request in the queue to update.
*/
index: number;

/**
* The new request to replace the existing request in the queue.
*/
request?: Request;
};

/**
* Model of a conflict request that needs to be deleted from the request queue.
*/
type ConflictRequestDelete = {
/**
* The action to take in case of a conflict.
*/
type: 'delete';

/**
* The indices of the requests in the queue that are to be deleted.
*/
indices: number[];

/**
* A flag to mark if the new request should be pushed into the queue after deleting the conflicting requests.
*/
pushNewRequest: boolean;

/**
* The next action to execute after the current conflict is resolved.
*/
nextAction?: ConflictData;
};

/**
Expand Down Expand Up @@ -97,7 +132,7 @@ type ConflictActionData = {
/**
* The action to take in case of a conflict.
*/
conflictAction: ConflictRequestReplace | ConflictRequestPush | ConflictRequestNoAction;
conflictAction: ConflictData;
};

/**
Expand All @@ -115,6 +150,11 @@ type RequestConflictResolver = {
* the ongoing request, it will be removed from the persisted request queue.
*/
persistWhenOngoing?: boolean;

/**
* A boolean flag to mark a request as rollbacked, if set to true it means the request failed and was added back into the queue.
*/
isRollbacked?: boolean;
};

/** Model of requests sent to the API */
Expand Down Expand Up @@ -147,4 +187,4 @@ type PaginatedRequest = Request &
};

export default Request;
export type {OnyxData, RequestType, PaginationConfig, PaginatedRequest, RequestConflictResolver, ConflictActionData};
export type {OnyxData, RequestType, PaginationConfig, PaginatedRequest, RequestConflictResolver, ConflictActionData, ConflictData};
Loading
Loading