Task #644 Refactor create and edit assignemnts functions by rolivares93 · Pull Request #20 · levante-framework/levante-firebase-functions

rolivares93 · 2025-11-05T13:57:53Z

Proposed changes

I refactored the functions responsible for the assignment creation to avoid the delay caused by firebase background functions.

Screen.Recording.2025-11-05.at.10.53.55.mov

Types of changes

What types of changes does this pull request introduce?

Bugfix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Refactor (non-breaking change that does not add functionality but makes code cleaner or more efficient)
Tests (new or updated tests)
Styles (changes to code styling)
CI (continuous integration changes)
Other (please describe below)

Additional Notes

Zio-4 · 2025-11-05T17:35:49Z

We need a way to emulate a complete atomic transaction. In other words, we need to guarantee that all users were assigned the assignment successfully. The current approach can fail for some users, while the rest would have the assignment. We want to avoid that. This also uses the previous approach of background triggers. We do not want eventual consistency. All users should be able to log in and play the assignment immediately once the admin creates it.

…d functions

…participants receive the assignment

Zio-4

functions/levante-admin/SYNCHRONOUS_ASSIGNMENT_SYNC.md

Zio-4 · 2025-11-12T23:04:12Z

Since this is a big PR and takes time to review, I'm gonna hold off until I get a response to the comments above.

functions/levante-admin/SYNCHRONOUS_ASSIGNMENT_SYNC.md

…onAssignments to createAssignments and updateAssignments respectively

functions/levante-admin/src/assignments/sync-assignments.ts

asengupta3 · 2025-12-01T14:29:40Z

functions/levante-admin/src/assignments/sync-assignments.ts

-          administrationData,
-          transaction
+          orgChunk,
+          administrationData


Do you think the rollbackAssignmentCreation needs to be chunked as well since the db supports some MAX_TRANSACTIONS? If not, I suggest we just simply pass usersToUpdate since we tested that rollbackAssignmentCreation does not affect users that have not been created. So it is safe to pass userIds that have no entries.

functions/levante-admin/src/upsertAdministration.ts

asengupta3 · 2025-12-01T14:37:24Z

functions/levante-admin/src/assignments/sync-assignments.ts

+    // If remainingUsersToRemove.length === 0, then these chunks will be of zero length
+    // and the entire loop below is a no-op.
+    for (const _userChunk of _chunk(remainingUsers, MAX_TRANSACTIONS)) {
+      await db.runTransaction(async (transaction) => {


Also I am not in favor of this pattern. What is going to happen here is, we are going to do sequential updates. Meaning we will update a set of users; wait and then update the next set of users, that adds time additively. Do you think firebase supports parallel requests (at least we should try it out)? In that case what I suggest pushing all the updateAssignmentForUsers and other async functions in an array and run Promise.all(array). That way the requests will be fired together and be faster.

asengupta3 · 2025-12-03T14:26:21Z

functions/levante-admin/src/assignments/sync-assignments.ts

+
+    if (mode === "add") {
+      totalUsersAssigned = usersToUpdate.length;
+      createdUserIds.push(...usersToUpdate);


Is there a reason we need to create a copy of usersToUpdate? Can't we simply pass that?

asengupta3 · 2025-12-03T14:28:14Z

functions/levante-admin/src/upsertAdministration.ts

+  try {
+    for (let i = 0; i < orgChunks.length; i++) {
+      const orgChunk = orgChunks[i];
+      const result = await updateAssignmentsForOrgChunkHandler({


We should follow the same Promise.all pattern here as well if we have verified that it works for the updateAssignmentsForOrgChunkHandler function

asengupta3 · 2025-12-03T14:28:53Z

functions/levante-admin/src/upsertAdministration.ts

+    });
+
+    for (const _userChunk of _chunk(remainingUsersToRemove, MAX_TRANSACTIONS)) {
+      await db.runTransaction(async (transaction) => {


Same promise.all here as well

asengupta3 · 2025-12-03T14:29:12Z

functions/levante-admin/src/upsertAdministration.ts

+
+  for (let i = 0; i < orgChunks.length; i++) {
+    const orgChunk = orgChunks[i];
+    const result = await updateAssignmentsForOrgChunkHandler({


Same Promise.all here as well.

asengupta3 · 2025-12-03T14:30:09Z

functions/levante-admin/src/assignments/assignment-utils.ts

+  if (orgChunk && administrationData && userIds.length > 0) {
+    try {
+      const { updateAdministrationStatsForOrgChunk } = await import(
+        "./on-assignment-updates.js"


Try to move this to the top as well

rolivares93 · 2025-12-17T18:27:13Z

CREATING ASSIGNMENT

CREATING.ASSIGNMENT.2025-Dec-17-2.mov

VIEWING ASSIGNMENT

VIEWING.ASSIGNMENT.2025-Dec-17-2.mov

Zio-4

It's hard to understand what a lot of the code is doing without going deep into the details like the "standardizeAdministrations" function for example.

Could you comments throughout the functions to make it clear what's happening like what the function is actually doing?

I was originally hoping we could completely refactor the create / edit assignment functions, but this will do for now.

Zio-4 · 2025-12-17T19:19:40Z

functions/levante-admin/src/assignments/sync-assignments.ts

-    });
+    }
+
+    return { success: false, userIds: createdUserIds, error };


If we're going to do this, then we need to update the frontend to check for this.

My take, since we're doing it atomically, all or nothing, we should just throw an error and then the frontend can say that user creation failed. Because there's no partial success, so we're not going to be showing rows of users that failed.

Zio-4 · 2025-12-17T19:27:31Z

functions/levante-admin/src/upsertAdministration.ts

+      if (usersToRemove.length !== 0) {
+        if (usersToRemove.length <= MAX_TRANSACTIONS) {
+          return removeOrgsFromAssignments(
+            usersToRemove,
+            [administrationId],
+            removedExhaustiveOrgs,
+            transaction
+          );
+        } else {
+          remainingUsersToRemove = usersToRemove;
+          return Promise.resolve(usersToRemove.length);
+        }
+      } else {
+        return Promise.resolve(0);
+      }


What is this section of code doing? Why are we resolving with the usersToRemove and then or resolving with zero? I think this should be rewritten to be more readable.

Zio-4 · 2025-12-17T19:37:25Z

functions/levante-admin/src/upsertAdministration.ts

+        if (!creatorUid) {
+          logger.error(
+            `Cannot sync assignments: administration ${newAdministrationId} has no createdBy field`,
+            { administrationId: newAdministrationId }
+          );
+          throw new HttpsError(
+            "internal",
+            "Administration document is missing creator information"
+          );


This shouldn't be possible because the creatorId will always be on the auth token when we first create the administration document, so it could never be missing it.

Zio-4 · 2025-12-17T19:38:18Z

functions/levante-admin/src/upsertAdministration.ts

+        if (!creatorUid) {
+          logger.error(
+            `Cannot sync assignments: administration ${newAdministrationId} has no createdBy field`,
+            { administrationId: newAdministrationId }
+          );
+          throw new HttpsError(
+            "internal",
+            "Administration document is missing creator information"
+          );
+        }


Not possible

Zio-4 · 2025-12-17T19:40:21Z

functions/levante-admin/src/upsertAdministration.ts

+      // If this is a new administration and sync failed, the error was already thrown
+      // and should have triggered rollback. Re-throw to prevent the function from
+      // returning successfully when assignments failed to be created.
+      const isNewAdministration = !administrationId;


Let's move this variable up a scope if we're going to reuse it here so we don't re-declare the same variables.

Zio-4 · 2025-12-17T20:08:14Z

Also important issues that Codex caught:

[P1] Rollback batch reused after commit fails beyond 500 users — assignments/assignment-utils.ts:204-214
add writes to a batch that has been committed, aborting the rollback and leaving many assignments/stat counters in place. To ensure rollback works for large cohorts, a new batch should be created each time a commit occurs.
[P1] Partial assignments not rolled back when org chunk transaction fails — /assignments/sync-assignments.ts:226-291
The new synchronous handler executes user chunks in Promise.all and only populates createdUserIds after all chunk transactions resolve. If any transaction rejects, Promise.all throws before that push runs, yet earlier transactions may already have succeeded; the catch path treats createdUserIds as empty and skips rollback. The caller then reports the chunk as failed but leaves successfully written assignments in place, breaking the advertised atomicity for new administrations. Track successes per chunk and roll them back on error.

rolivares93 self-assigned this Nov 5, 2025

rolivares93 added 6 commits November 10, 2025 10:48

added build:watch to help development

84806b9

refactored assignment creation to avoid delay with firebase backgroun…

d379cae

…d functions

added README to explain the changes with assignment creation

5626283

added rollback function to revert assignment creation if not all the …

b39df01

…participants receive the assignment

deleting the new assignment doc if the process is reverted

8d1b317

updated README file

0ee73ee

rolivares93 force-pushed the task-644/refactor-create-and-edit-assignemnts-functions branch from 5b98fde to 0ee73ee Compare November 12, 2025 13:41

Zio-4 reviewed Nov 12, 2025

View reviewed changes

functions/levante-admin/SYNCHRONOUS_ASSIGNMENT_SYNC.md Outdated Show resolved Hide resolved

functions/levante-admin/SYNCHRONOUS_ASSIGNMENT_SYNC.md Outdated Show resolved Hide resolved

functions/levante-admin/SYNCHRONOUS_ASSIGNMENT_SYNC.md Outdated Show resolved Hide resolved

Zio-4 reviewed Nov 12, 2025

View reviewed changes

functions/levante-admin/SYNCHRONOUS_ASSIGNMENT_SYNC.md Outdated Show resolved Hide resolved

rolivares93 added 3 commits November 13, 2025 14:36

removed org chunk limit to work on all of them

90f3226

updated the README file to match the new sync process

96784a5

renamed syncNewAdministrationAssignments and syncModifiedAdministrati…

71c26a1

…onAssignments to createAssignments and updateAssignments respectively

asengupta3 reviewed Dec 1, 2025

View reviewed changes

functions/levante-admin/src/assignments/sync-assignments.ts Outdated Show resolved Hide resolved

asengupta3 reviewed Dec 1, 2025

View reviewed changes

functions/levante-admin/src/assignments/sync-assignments.ts Outdated Show resolved Hide resolved

asengupta3 reviewed Dec 1, 2025

View reviewed changes

functions/levante-admin/src/upsertAdministration.ts Outdated Show resolved Hide resolved

asengupta3 reviewed Dec 1, 2025

View reviewed changes

functions/levante-admin/src/upsertAdministration.ts Outdated Show resolved Hide resolved

asengupta3 reviewed Dec 1, 2025

View reviewed changes

functions/levante-admin/src/upsertAdministration.ts Outdated Show resolved Hide resolved

asengupta3 reviewed Dec 1, 2025

View reviewed changes

rolivares93 added 3 commits December 2, 2025 11:52

moved imports to the top of the file

4fa0e8b

removed redundant chunk of code

336d4e1

running transactions in parallel

6b09a21

rolivares93 requested review from Zio-4 and asengupta3 December 2, 2025 14:59

asengupta3 reviewed Dec 3, 2025

View reviewed changes

rolivares93 added 3 commits December 17, 2025 12:17

moved import to the top of the file

2ee7fce

removed useless variable

b1b4da6

small refactoring on parallel functions

7ee8357

rolivares93 requested a review from asengupta3 December 17, 2025 17:37

Zio-4 reviewed Dec 17, 2025

View reviewed changes

asengupta3 mentioned this pull request Feb 10, 2026

Harden assignment sync to prevent disappearing assignments #43

Open

4 tasks

Conversation

rolivares93 commented Nov 5, 2025

Proposed changes

Types of changes

Additional Notes

Uh oh!

Zio-4 commented Nov 5, 2025

Uh oh!

Zio-4 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Zio-4 commented Nov 12, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

asengupta3 Dec 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rolivares93 commented Dec 17, 2025

Uh oh!

Zio-4 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Zio-4 commented Dec 17, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

asengupta3 Dec 1, 2025 •

edited

Loading