Skip to content

Record created/lastModified audit info on surveys - #2642

Draft
rfontanarosa wants to merge 2 commits into
masterfrom
rfontanarosa/1014/import-geojson-update-import-function-to-always-include-auditinfo
Draft

rfontanarosa wants to merge 2 commits into
masterfrom
rfontanarosa/1014/import-geojson-update-import-function-to-always-include-auditinfo

Conversation

@rfontanarosa

Copy link
Copy Markdown
Collaborator

closes #1014

@gemini-code-assist gemini-code-assist Bot left a comment

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.

Code Review

This pull request introduces created and lastModified audit info fields to the Survey protobuf message and integrates them across the web client and Cloud Functions. Specifically, the web client now generates audit info when creating, updating, or copying surveys, and a Cloud Function handler corrects client-guessed server timestamps on write. The review feedback correctly identifies two important issues: first, calling surveyToDocument without auditInfo can lead to data loss if used with a non-merging setDoc operation; second, updateSurveyAuditInfo in the datastore can throw a runtime error if it attempts to update Firestore with an empty object, which should be guarded.

Comment on lines 101 to 105
export function surveyToDocument(
surveyId: string,
survey: Partial<Survey>
survey: Partial<Survey>,
auditInfo?: { created?: Pb.IAuditInfo; lastModified?: Pb.IAuditInfo }
): DocumentData {

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.

high

When surveyToDocument is called without the auditInfo parameter, the returned document data completely omits the created and lastModified fields. While this is safe for update operations, any caller using setDoc without { merge: true } (such as addOrUpdateSurvey in DataStoreService) will overwrite the entire document and inadvertently delete any existing created and lastModified audit info from Firestore.

Consider updating addOrUpdateSurvey to preserve these fields, or ensure that surveyToDocument can extract them if they are present in the survey model.

Comment on lines +335 to +342
async updateSurveyAuditInfo(surveyId: string, surveyDoc: DocumentData) {
const surveyRef = this.db_.doc(survey(surveyId));
const update: DocumentData = {};
if (sv.created in surveyDoc) update[sv.created] = surveyDoc[sv.created];
if (sv.lastModified in surveyDoc)
update[sv.lastModified] = surveyDoc[sv.lastModified];
await surveyRef.update(update);
}

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.

medium

If surveyDoc does not contain either sv.created or sv.lastModified, the update object will remain empty. Calling surveyRef.update({}) with an empty object can throw a runtime error in the Firestore SDK (e.g., Update() requires at least one field of update data).

We should guard the update call by checking if there is at least one field to update.

  async updateSurveyAuditInfo(surveyId: string, surveyDoc: DocumentData) {
    const surveyRef = this.db_.doc(survey(surveyId));
    const update: DocumentData = {};
    if (sv.created in surveyDoc) update[sv.created] = surveyDoc[sv.created];
    if (sv.lastModified in surveyDoc)
      update[sv.lastModified] = surveyDoc[sv.lastModified];
    if (Object.keys(update).length > 0) {
      await surveyRef.update(update);
    }
  }

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 14.28571% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.22%. Comparing base (a3e8936) to head (08715d4).
⚠️ Report is 3 commits behind head on master.

Files with missing lines Patch % Lines
.../src/app/services/data-store/data-store.service.ts 0.00% 12 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2642      +/-   ##
==========================================
- Coverage   65.22%   65.22%   -0.01%     
==========================================
  Files         121      121              
  Lines        3031     3042      +11     
  Branches      463      469       +6     
==========================================
+ Hits         1977     1984       +7     
- Misses        986      987       +1     
- Partials       68       71       +3     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

This branch has not been deployed

No deployments
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.

[Import GeoJSON] Update import function to always include auditInfo

2 participants