feat: contacts import ocs - #61277
Conversation
9e175b9 to
7553769
Compare
|
|
||
| use JsonSerializable; | ||
|
|
||
| interface ImportEvent extends JsonSerializable { |
There was a problem hiding this comment.
What value does this interface add of it has no methods?
There was a problem hiding this comment.
This is a common type for ImportCountEvent and ImportObjectEvent, its used to stream events all the way to the UI while processing.
There was a problem hiding this comment.
Got it. For simplicity, could you just use JsonSerializable in the few areas you use ImportEvent right now?
There was a problem hiding this comment.
Hmm, I think I would prefer the current way to make it more clear what's being expected from a readability standpoint 🤔
There was a problem hiding this comment.
This PR has to be backported. Can we keep it simple?
There was a problem hiding this comment.
You can add the YAGNI abstraction to master. But for the backportable bit let's skip it
There was a problem hiding this comment.
This PR has to be backported. Can we keep it simple?
Ahh, yeah, I didn't had that in mind. So, I'll agree with you 😄
There was a problem hiding this comment.
You can add the YAGNI abstraction to master. But for the backportable bit let's skip it
I agree with this. Lets do this.
|
There are still some issues 🤔 The Call to undefined method OCA\\ContactsInteraction\\AddressBook::getResourceId()Memory exhaustedAnd the following warning is being thrown sometimes at the end of the import: Header already modified |
|
Hey David, Thanks for testing! Looks like some of these are not related to my PR.
|
c171464 to
90e83f0
Compare
6de1791 to
3b8c00a
Compare
|
So I tracked down the memory exhaustion error and its not related to this PR, its happening at the request parsing level. Therefore unrelated and out of scope |
3b8c00a to
d7ed0f7
Compare
23677e1 to
dcc65de
Compare
|
Tested thru nextcloud/contacts#5386, works for me with basic files but I didn't try breaking it |
dcc65de to
857859f
Compare
DerDreschner
left a comment
There was a problem hiding this comment.
🐘 But tested it a month ago and was working there beside some issues that might be on my side, so 😄
|
General FYI: Sebastian wants to fix the CI once #55178 is merged |
857859f to
77353d2
Compare
9e80cee to
c4b321e
Compare
Signed-off-by: SebastianKrupinski <krupinskis05@gmail.com>
c4b321e to
329a7a6
Compare
Yup, already done, green like the 18th on the Canadian Open |
Summary
Checklist
3. to review, feature component)stable32)AI (if applicable)