-
Notifications
You must be signed in to change notification settings - Fork 24
chore: move db models to wire data package - WPB-19688 #3521
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
base: develop
Are you sure you want to change the base?
Conversation
…om:wireapp/wire-ios into feat/add-cellsState-prop-to-ZMConversation
…o properly load db models
Test Results 9 files 226 suites 5m 52s ⏱️ For more details on these failures, see this check. Results for commit 77fac82. ♻️ This comment has been updated with latest results. |
wire-ios-data-model/Resources/zmessaging.xcdatamodeld/zmessaging2.80.0.xcdatamodel/contents
Show resolved
Hide resolved
WireData/Sources/WireData/Schema/zmessaging.xcdatamodeld/zmessaging2.80.0.xcdatamodel/contents
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.
thanks for extensive description! One more thing to test before merging would be upgrade from a playground build to double check there's no pb
|
Issue
This PR moves the
zmessaging
andZMEventModel
database models fromWireDataModel
framework toWireData
package ensuring that the managed objects that were previously pointing to theCurrent Product Module
now points toWireDataModel
(where still reside their related subclasses).Note: there was no need to re-generate the resources (i.e
store2-129-0.wiredatabase
) for the tests inWireDataModel
as the tests were passing.Sam's extra comments
I've assigned this PR to myself as @jullianm is out this week. Some things I've learnt which I didn't know before regarding the classes module:
This can have 3 states:
Global Namespace
is required for managed object subclasses that are obj-c or those that are used from obj-c. This is because obj-c uses a global namespace (hence the need to add prefixes).Current Product Module
is likeCustom
but automatically sets the target based on where the database models reside. CurrentlyCurrent Product Module
resolves toWireDataModel
but with this PR we move the database models toWireData
while keeping the classes inWireDataModel
. This means that we have to updates settings ofCurrent Product Module
toWireDataModel
.Looking at this PR I was surprised that it updates each version of
zmessaging.xcdatamodeld
, not just2.129
but2.128
,2.127
, etc. I now realize that this is because some of our migration tests depend on our managed object subclasses with older database versions. For example,DatabaseMigrationTests_Conversations.testThatItPerformsMigrationFrom106_deleteConversationCascadesToParticipantRole()
. I think this is a little problematic as there is no reason why the current version ofParticipantRole
is compatible with version2.106.0
for example. It would be better to use just NSManagedObject with KVC. However it's probably best to leave things how they are.Beyond @jullianm's work I added one small additional test to assert that each managed object subclass of the latest data model can be instantiated.
Testing
WireDataModel
andWireSyncEngine
are still passingChecklist
[WPB-XXX]
.UI accessibility checklist
If your PR includes UI changes, please utilize this checklist: