Improve settings

[reinhardt]

Now that we have two messages of the day (#202), it is a little awkward to query which of them is disabled. I have to pass in all the known values to the query:

Session.query(NewsletterSetting.value).filter(
    NewsletterSetting.account_id == account.id,
    NewsletterSetting.value.in_(
        [message["disabled_key"] for message in self.all_messages]
    ),
)

I would rather do:

Session.query(NewsletterSetting.value).filter(
    NewsletterSetting.account_id == account.id,
    NewsletterSetting.type == "motd-disabled"
    ),
)

Furthermore, the new setting has nothing to do with the newsletter. I would argue that the old setting also isn't really about the newsletter but about the message of the day. How about instead of specific settings tables we use only one table (e.g. AccountSetting), but with an additional type column to allow specifying what the setting is about?

Something like:

class AccountSetting(model.BaseObject):
    __tablename__ = "account_setting"

    id = schema.Column(types.Integer(), primary_key=True, autoincrement=True)
    account_id = schema.Column(
        types.Integer(),
        schema.ForeignKey(model.Account.id, onupdate="CASCADE", ondelete="CASCADE"),
        nullable=False,
    )
    type = schema.Column(types.String(512), nullable=False)
    value = schema.Column(types.String(512), nullable=False)

This would also allow migrating the newsletter subscriptions to the new table.

AccountSetting(account_id=12345, type="newsletter_subscription", value="fr/bakery/bakery")
AccountSetting(account_id=12345, type="newsletter_subscription", value="fr")

Note that this should probably live in Euphorie instead of osha.oira.

[reinhardt]

@ale-rt, any thoughts about that?

[ale-rt]

About renaming the table, if you want to do that, do that.
Please, remember to write a migration step.

It looks to me like you want to towards a per-account registry.
What about using key/value rather than type/value.
So far I do not see it really necessary because we just have boolean values, so theoretically one column is enough.

My gut feeling is that a migration to the NewsletterSetting to your proposed AccountSetting might cause more headaches than the one it solves.
It is too smart ;)

[reinhardt]

So far I do not see it really necessary because we just have boolean values, so theoretically one column is enough.

As explained in the ticket description we now have a setting that should hold a list of disabled messages.

[ale-rt]

So far I do not see it really necessary because we just have boolean values, so theoretically one column is enough.

As explained in the ticket description we now have a setting that should hold a list of disabled messages.

I do not understand this comment. What is its purpose?