search

Wingysam / Christmas-Community

Christmas lists for families
GNU Affero General Public License v3.0
245 stars 41 forks source link

fix: solve ???/??? for logged in user's wishlist #105

Closed cj13579 closed 1 week ago

cj13579 commented 10 months ago

This proposed change looks to resolve the '???/???' stuff that's seen by the logged in user. There's also a "bonus" change of setting the VS Code default node port to make things more consistent with if you start the application using npm start.

This change removes a block of code that treated the logged in user differently from other users. I don't think that this is necessary. With this change, all lists are processed with the same logic. I've added some tidy-up to the language files too as the WISHLISTS_COUNTS_SELF string is no longer required.

If accepted this issue closes #88.

TheTechmage commented 10 months ago

Reading through the changes, I'm not sure how this solves "hiding the count of items that other users have added to your wishlist" concern that was brought up in #88

So if User A adds 8 items to their list, and User B adds 4 more, User A can see that there's 12 items on their list, including the items that User B has added. When User A goes to their wishlist, they will not see the additional changes and will only see 8 items instead of 12. If I understand the wishlistDetails method correctly, User A could also view the items that User B added to User A's wishlist, but only from the user list page.

cj13579 commented 10 months ago

That's fair. It shows the total of the list. I still think that's better than ???/??? but I can have a think about filtering this list too.

cj13579 commented 10 months ago

I've pushed another change (https://github.com/Wingysam/Christmas-Community/pull/105/commits/ea8a00090ae68bab1bdd3c3285605046941c2173) that now means it only shows the total of the items that you've added yourself.

Wingysam commented 9 months ago

I think it's best to calculate how many items you've added to your own list in the JS route handler and pass that as a property to the pug template rather than calculating it in pug.

cj13579 commented 9 months ago

I think it's best to calculate how many items you've added to your own list in the JS route handler and pass that as a property to the pug template rather than calculating it in pug.

Yep, that makes sense. With this commit (https://github.com/Wingysam/Christmas-Community/pull/105/commits/152a562e8308baebceae33237b362f10fb2632ec) I've updated the totals() function to calculate self added items. I've modified the view to call that function for each wish list once and then use the metrics needed as appropriate. For deployments with large numbers of lists, a performance improvement should be seen since the calculations for pledge counts etc. is only done once where it was done many times before.

cj13579 commented 9 months ago

I feel like the files change on this branch are all weird and I'm worried that we might be introducing some regressions. How do you feel about closing this and I'll open a new one cherry-picking the relevant commits.