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

[wip] db-changes user preferences to table/columns #1467

Open
wants to merge 10 commits into
base: master
from

Conversation

@luceos
Copy link
Member

commented Jun 19, 2018

This includes the user preferences changes needed for beta 8. As requested this has been split up from (and depends on) #1344

  • add drop and reinstate statement migration, see d9b357c

The migrations contained in this PR have to run in-between the migrations inside the PR #1344 ; the users.preferences column is dropped inside a migration of that parent PR.

@luceos luceos added the Database label Jun 19, 2018

@luceos luceos added this to the 0.1.0-beta.8 milestone Jun 19, 2018

@luceos luceos self-assigned this Jun 19, 2018

@luceos luceos changed the title moved user preferences to new branch [wip] db-changes user preferences to table/columns Jun 19, 2018

@luceos luceos referenced this pull request Jun 19, 2018

@luceos luceos removed this from the 0.1.0-beta.8 milestone Jun 22, 2018

@franzliedke franzliedke removed the Database label Jul 21, 2018

@franzliedke franzliedke changed the base branch from 1236-database-changes to master Sep 16, 2018

@franzliedke

This comment has been minimized.

Copy link
Member

commented Sep 16, 2018

Re-targeted this to master.

@franzliedke franzliedke added this to the 0.1.0-beta.9 milestone Dec 12, 2018

@franzliedke

This comment has been minimized.

Copy link
Member

commented Dec 12, 2018

@luceos Can you rebase this on the latest master, please?

luceos added some commits Mar 8, 2019

Apply fixes from StyleCI
[ci skip] [skip ci]
removed references to preferences column, now we need to refactor how…
… notification ppreferences is integrated into the current app
Apply fixes from StyleCI
[ci skip] [skip ci]
@luceos

This comment has been minimized.

Copy link
Member Author

commented Jul 10, 2019

@flarum/core we need to decide how to move forward with this; we decided back then to store notification preferences inside their own column and (I assume) other preferences should be stored by the extension on the user table as a separate column. I'm curious whether it might be wiser to have a user_preferences table instead that combines all preferences (again but as a table instead of a json column).

@luceos luceos requested a review from franzliedke Aug 9, 2019

@franzliedke

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

Toby strongly advocated using columns on the users table in his original proposal:

NO Entity-Attribute-Value tables. See here and here.


I tend to agree, especially as the use-case seems to be limited.

My proposal:

  • New table for notification_preferences, that's a real domain entity, not a super-generic Entity-Attribute-Value table
  • Move core preferences (locale and discloseOnline - are there others?) to dedicated columns
  • Leave the column for BC - we can consider marking it deprecated and recommend using separate columns.

By the way: JSON datatype may be useful if we decide to keep the column forever.

@franzliedke
Copy link
Member

left a comment

It seems like you forgot the locale preference?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.