Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multi-user Support #59

Open
dohsimpson opened this issue Feb 8, 2025 · 13 comments · May be fixed by #60
Open

Multi-user Support #59

dohsimpson opened this issue Feb 8, 2025 · 13 comments · May be fixed by #60

Comments

@dohsimpson
Copy link
Owner

Add multi user support as described in #9

@dohsimpson
Copy link
Owner Author

dohsimpson commented Feb 8, 2025

For anyone who want to test out the new feature. Give it a try and let me know your feedbacks:

Branch: multiuser
Docker image: dohsimpson/habittrove:dev

NOTE that this is a breaking change, requiring a new env var to be set (see README). Please backup your data/ directory in case you want to rollback.

@AsocPro
Copy link

AsocPro commented Feb 8, 2025

Some initial feedback this change now seems to require https. When running under http HabitTrove no longer loads with the following error: TypeError: crypto.randomUUID is not a function
when running under http crypto stuff isn't available as far as I know.

I added a simple https proxy to put things and the interface loaded but when I got to pressing login I started getting other certificate errors in the console and a 500 error on the request sent when pressing the login button. I don't currently have certificate stuff set up to easily get proper ssl certs set up on this server that I have available to test currently. I can work on getting that set up and then I can come back to this and give you more feedback.

@dohsimpson
Copy link
Owner Author

Interesting, I believe this crypto.randomUUID would come from when the diff here when it generates a user id for the initial admin user. But this shouldn't be new because this function already exists before this version (e.g. here).

I could be wrong but I don't think the error is related to the http certs. crypto is a node.js built-in function, could it be you are using a earlier version of node? (ref)

@AsocPro
Copy link

AsocPro commented Feb 9, 2025

I am running the dohsimpson/habittrove:dev docker container which is node v18.20.6. As per the Mozilla web dev docs crypto is only available in secure contexts. See note at the top of this page: https://developer.mozilla.org/en-US/docs/Web/API/Web_Crypto_API
It does seem as if the crypto.randomUUID() is called by the browser side of things as I could set some breakpoints and the page would load fine right up until it got to the code calling the crypto libs.

I’m not the most familiar with node apps so forgive me if I’m wrong or missing nuance here but it appears that the actions/data.ts file that’s has had the crypto.randomUUID call the whole time is on the server side (it has the ‘use server’ directive at the top) while the lib/types.ts doesn’t have that same directive at the top of the file which I assume is where the browser is having issues where before that call wasn’t exposed to the browser. When I looked at it in the browser dev tools it is somewhat minified and/or I’m missing a good way to correlate exactly which file it’s failing on so I’m not positive but this is just my guess without having taken the time to build/run the app in a dev environment to debug it directly.

@dohsimpson
Copy link
Owner Author

yeah, that makes sense. types.ts is imported by both server side and client side. And since crypto is a node only module, importing from client side would cause a problem.

In particular, I think it might be the import here that throw the error.

@dohsimpson
Copy link
Owner Author

I replaced the crypto.randomUUID function with the browser-compatible uuid library, which should fix this issue. Feel free to try out the latest docker image.

@AsocPro
Copy link

AsocPro commented Feb 9, 2025

That’s working now! Thanks! From a quick initial test it is looking great! I’ll dive in deeper and get you some more detailed feedback.

@AsocPro
Copy link

AsocPro commented Feb 10, 2025

Overall it’s a really great start. The overall user experience of multiple users is really good and the feature set is solid! Here are some more detailed findings though.

Shared item Coin event issues

There seem to be some inconsistencies with how coins are handled when it comes to shared items. My assumption would have been that the item state was shared but the redemption or completion of items would only affect the balance of the user clicking the button. The following things came up that didn’t seem to act as I would have expected.

  • Shared tasks when undo by another user it doesn’t undo the earned coins. Also if undone and redone the coins aren’t counted unless it was the original user to complete the task. It may be nice if one user completed a task/habit to only allow them to undo the completion. Not sure here though what is the best workflow to enforce.
  • Habits Tasks and Wishlist items shared by Admin users completed/redeemed by non admin users affect the coin balance of the Admin user. Items shared from a non Admin user to admin users don’t appear to have this same behavior.
  • The wishlist redeem button for wishlist redemption items shared from an Admin account can be disabled if the admin account doesn’t have enough coins to redeem the item even if the current user doesn’t have bough coins to redeem the item.

Password related findings

  • Short passwords shorter than 4 characters fail without a clear error message.
  • It would be nice to be able to have users that don’t have password just for ease of use in situations where passwords aren’t really needed.

Random nitpicks

  • Habits and Tasks are under the same permission. This is probably fine but it isn’t explicitly worded in the settings page.
  • Permissions are working well. It would be nice to have the edit and create buttons hidden when a user doesn’t have the permissions for those actions.
  • The layout of the permission toggles spread the label apart from the toggle and it would be easier visually to correlate which toggle is tied to what if they were right next to the label.
  • In the permissions section the labels and section headers even though they are different sizes they seem to flow together and it feels more like many lines of text instead of three distinct sections.

@dohsimpson
Copy link
Owner Author

dohsimpson commented Feb 13, 2025

latest commit and dev image addressed the following:

  • Shared tasks when undo by another user it doesn’t undo the earned coins. Also if undone and redone the coins aren’t counted unless it was the original user to complete the task. It may be nice if one user completed a task/habit to only allow them to undo the completion. Not sure here though what is the best workflow to enforce.

  • Habits Tasks and Wishlist items shared by Admin users completed/redeemed by non admin users affect the coin balance of the Admin user. Items shared from a non Admin user to admin users don’t appear to have this same behavior.

  • The wishlist redeem button for wishlist redemption items shared from an Admin account can be disabled if the admin account doesn’t have enough coins to redeem the item even if the current user doesn’t have bough coins to redeem the item.

  • Short passwords shorter than 4 characters fail without a clear error message.

  • It would be nice to be able to have users that don’t have password just for ease of use in situations where passwords aren’t really needed.

  • Habits and Tasks are under the same permission. This is probably fine but it isn’t explicitly worded in the settings page.

  • Permissions are working well. It would be nice to have the edit and create buttons hidden when a user doesn’t have the permissions for those actions.

  • The layout of the permission toggles spread the label apart from the toggle and it would be easier visually to correlate which toggle is tied to what if they were right next to the label.

  • In the permissions section the labels and section headers even though they are different sizes they seem to flow together and it feels more like many lines of text instead of three distinct sections.

  • fix CHANGELOG

@dohsimpson
Copy link
Owner Author

I guess one thing missing is admin doesn't have a way to view the transactions of other users. Probably want to fix this before release.

@AsocPro
Copy link

AsocPro commented Feb 13, 2025

I guess one thing missing is admin doesn't have a way to view the transactions of other users. Probably want to fix this before release.

Giving everything a good run through currently and I’ll report back more details but on this note the coins transactions page does show all transactions by all users when logged in as admin. It has the indicator with who did each transaction. Are you thinking like a view to see their daily habit status or summaries of how many coins users have?

I’m trying to think through my use cases and for the most part it’s good enough for me to switch user into the user I want to look at. Maybe an impersonate button would be nice for password enabled users?

The UI/UX is really good as is and I don’t know that I would want it polluted with too much extra information in the same interface but maybe there is room for a separate page for advanced administration or something.

@AsocPro
Copy link

AsocPro commented Feb 13, 2025

but on this note the coins transactions page does show all transactions by all users when logged in as admin. It has the indicator with who did each transaction.

Ignore my comment here about it being there. It was there previously but I hadn’t updated to the very latest before commenting. Which was silly of me. On the latest it is not showing transactions if other users.

Things are looking really solid. Just a few notes now I’ve walked through the different error cases seen before:

  • When creating a user if disable password is checked the user cannot be created. A password is required to be entered before the disable password box is checked.
    In the edit user dialog after an account is created it works correctly. It’s just the initial account creation that has this issue.
  • Just a note for documentation: in the first iteration the admin user didn’t require a password on initial setup. This behavior seems to have changed to use the password “admin”. This is fine and was an easy default to guess but it should be documented in the readme.
  • The new permissions layout is great. Very clear exactly what setting correlates with which checkbox.
  • On completed shared tasks/habits it would be nice if it showed visually who completed the task/habit.
  • The shared task (and habit/wishlist) feature works pretty well for adding tasks to accounts without privs to add and being able to track the completion of said tasks. It would be nice to be able to do manual coin transactions for a specific user from an admin user. Currently the only way I have seen to do manual transactions for accounts that are would be give the user coins write permissions log in as that user do the transaction and then log back into admin and change back the privs.

@dohsimpson
Copy link
Owner Author

  • When creating a user if disable password is checked the user cannot be created. A password is required to be entered before the disable password box is checked. In the edit user dialog after an account is created it works correctly. It’s just the initial account creation that has this issue.
    This is fixed in the new commit.
  • we still have an issue where disable password and leaving it blank in createUser view has the same effect. We should just not have disable password toggle, and have a placeholder text saying leave empty to disable password.
  • Just a note for documentation: in the first iteration the admin user didn’t require a password on initial setup. This behavior seems to have changed to use the password “admin”. This is fine and was an easy default to guess but it should be documented in the readme.
    Thanks, this is no longer an issue, as before it was treating "admin" as the default password and allow passwordless login when it's using the default password, now empty password allows passwordless login.
  • On completed shared tasks/habits it would be nice if it showed visually who completed the task/habit.
    this will be tough, as currently, completing a task only add a timestamp with no user information logged. This info is logged in the coin transaction though, but cross referencing is not very straightforward.
  • The shared task (and habit/wishlist) feature works pretty well for adding tasks to accounts without privs to add and being able to track the completion of said tasks. It would be nice to be able to do manual coin transactions for a specific user from an admin user. Currently the only way I have seen to do manual transactions for accounts that are would be give the user coins write permissions log in as that user do the transaction and then log back into admin and change back the privs.
    We should be able to do this, however, this might needs a separate UI, as the current coins page shows the balance and transactions of the current user only (even if they are admin).

Overall, thanks so much for the through review!! Really appreicate your help in opening the issue for this feature and making this release better! @AsocPro

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 a pull request may close this issue.

2 participants