refactor: remove `user.id` use and rework vote logic #401

iuioiua · 2023-07-28T02:21:34Z

Vote logic was challenging to deal with, hence the rework. The most involved part of this PR is the migration script. In summary:

All notifications will be deleted, which, IMO, is ok.
User entries are untouched.
Items and votes entries are migrated according to their new respective interfaces.

Reviews, please pay close attention to the migration script. It aims to preserve item, vote and user data. This script has worked multiple times on my local machine, but I still want it checked carefully. To test:

On the main branch, run deno task db:reset and then deno task db:seed.
Run deno task start, sign in, vote and comment on random items, noting the changes.
Switch to this branch and run deno run -A --unstable tools/migrate_kv.ts.
Run deno task start and ensure all data is preserved.

Please review, if willing and able, @brunocorrea23 and @mbhrznr.

Closes #400

iuioiua

Some comments to understand the changes. There are other minor changes throughout this PR which are self-explanatory.

iuioiua · 2023-07-31T01:09:50Z

routes/_404.tsx

Passing-by changes.

iuioiua · 2023-07-31T01:10:04Z

routes/_500.tsx

Passing-by changes.

iuioiua · 2023-07-31T01:16:33Z

tools/migrate_kv.ts

+  await Promise.all(promises1);
+
+  // Items
+  const promises2 = [];


Both arrays of promises, promises1 and promises2, must resolve sequentially. Vote migrations cover all items that have votes. Item migrations cover all remaining items that have no votes.

Using Promise.all() is optimal for performance.

iuioiua · 2023-07-31T01:22:05Z

tools/migrate_kv.ts

Note to self: voted_items_by_user and voted_users_by_item indexes should be deleted once this PR is merged, as they are no longer used.

iuioiua · 2023-07-31T01:22:59Z

tools/migrate_kv.ts

+  const [creatorUserRes, voterUserRes] = await kv.getMany<OldUser[]>([
+    ["users", oldItem.userId],
+    ["users", voterUserId],
+  ]);
+  assertIsEntry(creatorUserRes);


The distinction between these two types of users must remain correct throughout this function.

iuioiua · 2023-07-31T01:24:16Z

tools/migrate_kv.ts

+  if (!res.ok) throw new Error(`Failed to set vote: ${vote}`);
+}
+
+async function migrateItem(oldItem: OldItem) {


This is run after migrateVote() and only takes care of those items with no votes.

mbhrznr · 2023-07-31T05:03:28Z

thank you for providing the comments alongside the changes, which helped a lot in understanding the broader context.
also thank you for providing the steps to get this going!

i was able to get through the steps w/o any issues.
resetting, seeding and running the migration worked like a charm.
the votes i initially added were also persisted and still existed after i run the migration script.

however i've noticed the following:

downvoting an upvoted item removes the vote from the counter, however on refresh the post is still marked as upvoted. this way an item can be downvoted to 0.
upvoting an item, which you already interacted with increments the counter and updates the color in an optimistic update, yet on refresh the vote is gone. initially i thought this could just be the default browser behavior of re-submitting the form, however the same happens whenever i navigate to the page, e.g. via click on the logo.
trying to upvote an existing item, which you haven't interacted with before, throws an AssertionError, which seems to be coming from the assertIsEntry check:
```
An error occurred during route handling or page rendering. AssertionError: Expected actual: null not to be: null: KV entry not found
```
initially upvoting any items which are created after the migration works as expected. downvoting that exact same item still shows the item as upvoted when i navigate to the page (see 1.).

i will also do a more thorough review of the code changes but wanted to drop a note about my findings asap.

iuioiua · 2023-08-03T22:48:28Z

I've fixed the migration script by removing what may be unneeded assertions in it. @kt3k and @mbhrznr, can you please re-run the migration script with the new changes? This likely fixes the issues outlined in the previous comment, as the vote migrations weren't completed.

kt3k

Ran migration on prod KV snapshot successfully and updated DB seems working without problem. LGTM

mbhrznr · 2023-08-04T14:23:38Z

@iuioiua thanks for the update!

just tested the functionality from resetting to seeding to voting and migrating.
yet it looks like i can still reproduce all of the findings in the initial comment.
did any of these occur on your end(s) as well?

iuioiua · 2023-08-06T21:51:54Z

I think the seed task may cause the issue you're experiencing. I just tested using the actual production data for the site, and it works fine! I'll run the migration and go from there. Thanks for your help, guys!

iuioiua added 5 commits July 28, 2023 12:20

refactor: remove user.id use

27b7638

tweaks

b52cace

passing by tweaks

03b00e5

work

ec8eced

work

8859501

iuioiua changed the title ~~refactor: remove user.id use~~ refactor: remove user.id use and rework vote logic Jul 31, 2023

iuioiua changed the title ~~refactor: remove user.id use and rework vote logic~~ refactor: remove user.id use and rework vote logic Jul 31, 2023

iuioiua requested a review from kt3k July 31, 2023 01:06

iuioiua marked this pull request as ready for review July 31, 2023 01:07

iuioiua commented Jul 31, 2023

View reviewed changes

iuioiua added 2 commits August 4, 2023 08:45

fixes

d856f3d

Merge branch 'main' into remove-user-id

db67edf

kt3k approved these changes Aug 4, 2023

View reviewed changes

iuioiua merged commit 617fd6f into main Aug 6, 2023

iuioiua deleted the remove-user-id branch August 6, 2023 21:52

This was referenced Aug 22, 2023

Vote count not working as expected #427

Closed

feat: <ItemsList /> with cursor-based pagination #445

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: remove `user.id` use and rework vote logic #401

refactor: remove `user.id` use and rework vote logic #401

iuioiua commented Jul 28, 2023 •

edited

Loading

iuioiua left a comment

iuioiua Jul 31, 2023

iuioiua Jul 31, 2023

iuioiua Jul 31, 2023

iuioiua Jul 31, 2023

iuioiua Jul 31, 2023

iuioiua Jul 31, 2023

mbhrznr commented Jul 31, 2023

iuioiua commented Aug 3, 2023 •

edited

Loading

kt3k left a comment

mbhrznr commented Aug 4, 2023

iuioiua commented Aug 6, 2023

refactor: remove user.id use and rework vote logic #401

refactor: remove user.id use and rework vote logic #401

Conversation

iuioiua commented Jul 28, 2023 • edited Loading

iuioiua left a comment

Choose a reason for hiding this comment

iuioiua Jul 31, 2023

Choose a reason for hiding this comment

iuioiua Jul 31, 2023

Choose a reason for hiding this comment

iuioiua Jul 31, 2023

Choose a reason for hiding this comment

iuioiua Jul 31, 2023

Choose a reason for hiding this comment

iuioiua Jul 31, 2023

Choose a reason for hiding this comment

iuioiua Jul 31, 2023

Choose a reason for hiding this comment

mbhrznr commented Jul 31, 2023

iuioiua commented Aug 3, 2023 • edited Loading

kt3k left a comment

Choose a reason for hiding this comment

mbhrznr commented Aug 4, 2023

iuioiua commented Aug 6, 2023

refactor: remove `user.id` use and rework vote logic #401

refactor: remove `user.id` use and rework vote logic #401

iuioiua commented Jul 28, 2023 •

edited

Loading

iuioiua commented Aug 3, 2023 •

edited

Loading