-
Notifications
You must be signed in to change notification settings - Fork 11
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
DBP API changes for WebUI #2146
Conversation
...ataBrokerProtection/Sources/DataBrokerProtection/DebugUI/DataBrokerDatabaseBrowserView.swift
Outdated
Show resolved
Hide resolved
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.
It is looking good 👍🏼 . I’ve added some comments. The most pressing one is the unique
on the new url column
LocalPackages/DataBrokerProtection/Sources/DataBrokerProtection/Model/DataBroker.swift
Show resolved
Hide resolved
...BrokerProtection/Sources/DataBrokerProtection/Database/DataBrokerProtectionDataManager.swift
Show resolved
Hide resolved
...kages/DataBrokerProtection/Sources/DataBrokerProtection/Operations/DataBrokerOperation.swift
Outdated
Show resolved
Hide resolved
LocalPackages/DataBrokerProtection/Sources/DataBrokerProtection/Services/EmailService.swift
Outdated
Show resolved
Hide resolved
...erProtection/Sources/DataBrokerProtection/Storage/DataBrokerProtectionDatabaseProvider.swift
Outdated
Show resolved
Hide resolved
do { | ||
if try database.tableExists(BrokerDB.databaseTableName) { | ||
try database.alter(table: BrokerDB.databaseTableName) { | ||
$0.add(column: BrokerDB.Columns.url.name, .text) |
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.
$0.add(column: BrokerDB.Columns.url.name, .text).unique()
I have another question: should we remove the uniqueness from the name
column? Is it possible for two brokers to have the same name but different URLs?
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.
Not an easy way to do this without some bigger changes. It's probably possible to create a tmp table, move things there, drop the current one and re-create with the unique property but I don't think it's worth the effort now.
https://www.sqlite.org/lang_altertable.html
The column may not have a PRIMARY KEY or UNIQUE constraint.
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.
Looks great 🎉 !!!
# Conflicts: # LocalPackages/DataBrokerProtection/Tests/DataBrokerProtectionTests/DataBrokerProtectionUpdaterTests.swift
Cherry picked from #2146 Task/Issue URL: https://app.asana.com/0/1204167627774280/1206480273655878/f **Description**: Add url property to data broker model, and changes result for WebUI call to support new UI designs
Closed in favor of #2233 |
Task/Issue URL: https://app.asana.com/0/1204167627774280/1206480273655878/f
Description:
Add url property to data broker model, and changes result for WebUI call to support new UI designs
Steps to test this PR:
The most important thing to test is the database migration.