-
-
Notifications
You must be signed in to change notification settings - Fork 732
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
feat: Filter Out Shopping Lists That Aren't Yours #3213
feat: Filter Out Shopping Lists That Aren't Yours #3213
Conversation
In the first few screenshots the "Show All" checkbox is slightly off-center (it's too high), but this is fixed (see the "Add to List" screenshot) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@michael-genson happy with everything on here.
As discussed on Discord, can you please add in a way for a user/group to set "show all" to true and have it persist.
Use case being a family where person X creates the list and person Y is always going to want to look at the list created by person X - and never really create their own list.
The checkbox uses user preferences now, so in your use-case you only need to toggle it once (per device, since it uses local storage) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🚀
What type of PR is this?
(REQUIRED)
What this PR does / why we need it:
(REQUIRED)
This PR filters out shopping lists that aren't yours on the frontend (with a checkbox to toggle all of them):
This is also added to the "add recipe to list" dialog:
To do this, shopping lists are now tied to users (i.e. I added a
user_id
column). When creating a list, whoever creates the list will become the owner (similar to recipes). Since we don't have this information during migration, we just pick an arbitrary admin user (or, if there is no admin user, any user). To accommodate this (and assigning lists in general) there's a new settings option on the list which lets you set the owner:This PR also adds alphabetical ordering to shopping lists, because currently the order is arbitrary (id).
Which issue(s) this PR fixes:
(REQUIRED)
N/A
Special notes for your reviewer:
I wrote all the populate user migration SQL by hand because I've given up on ORM during migrations.
Testing
(fill-in or delete this section)
Modified existing tests to check for the
user_id
field. I also confirmed that one of our existing backup-restore tests includes exclusively non-admin users, so that SQL is tested (it failed originally because of a typo).