feat: handleOutdatedSuggestions gracefully #671
Conversation
|
This PR will trigger a minor release when merged. |
| @@ -74,18 +108,20 @@ export async function retrieveAuditById(dataAccess, auditId, log) { | |||
| export async function syncSuggestions({ | |||
| opportunity, | |||
| newData, | |||
| context, | |||
| buildKey, | |||
| mapNewSuggestion, | |||
| log, | |||
| }) { | |||
| const newDataKeys = new Set(newData.map(buildKey)); | |||
| const existingSuggestions = await opportunity.getSuggestions(); | |||
There was a problem hiding this comment.
while already there, with this change, this will possibly cause performance issues as the list of suggestions grows forevermore. at the very least a status filter should be employed. in fact the handling of outdated suggestions could be implemented as a bulk-operation as part of the SuggestionCollection in data access. We should not have use of data access queries in an unbounded fashion.
There was a problem hiding this comment.
We have three operations here:
-
handleOutdatedSuggestion
This method targets only 'NEW' and 'IN_PROGRESS' suggestions, having bounded query with status filter makes sense here. -
updateExistingSuggestion :
This method targets suggestions of all statuses. No bounded query can be written for status filter. [Based on our assumption that even FIXED or OUTDATED suggestion can be reopened in future] -
createNewSuggestion :
This method also requires information for all the suggestions irrespective of the statuses. No bounded query here as well. [Based on our assumption that even FIXED or OUTDATED suggestion can be reopened in future]
Bounding the operations 2 & 3 to 'NEW' and 'IN_PROGRESS' suggestions could work but as a consequence we would have to allow multiple suggestions with the same dataKeys in the db. This means that instead of reopening an existing FIXED or OUTDATED suggestion, we will create 'NEW' suggestions if the audit find the same issue again.
Note: Having the ability to query using the dataKeys would help us write bounded queries very efficiently for all the operations. But i think it is not possible yet.
@solaris007 Let me know what are you thoughts on this.
| SuggestionDataAccess.STATUSES.OUTDATED, | ||
| ].includes(existing.getStatus())) { | ||
| log.warn('Resolved suggestion found in audit. Possible regression.'); | ||
| existing.setStatus(SuggestionDataAccess.STATUSES.NEW); |
There was a problem hiding this comment.
there's already a Suggestion.bulkUpdateStatus method...
There was a problem hiding this comment.
I believe setStatus does not save the suggestion but only updates the entity locally. Save operation is performed later at L146.
Problem:
sync suggestions deletes outdated suggestions from the db.
Proposed solution:
For fix history phase 1, we are proposing to retain the suggestions and mark them 'OUTDATED' if they are not found in the latest audit run.
Changes:
Please ensure your pull request adheres to the following guidelines:
Related Issues
Thanks for contributing!