-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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
feature: mail provider backend #45383
Conversation
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.
Psalm found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
TODO: Separate attachment handling for local files (saved on the system) and streamed files (generate in memory) |
b1b1fe5
to
2187c7c
Compare
TODO: unit tests |
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! |
2ee01d0
to
43d6666
Compare
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 sane
43d6666
to
81763fb
Compare
999176d
to
b1f6e45
Compare
ff4903b
to
e7d0dfc
Compare
Each of the above methods can return null. For example, IAttachment: What's the use case for having an attachment object with a name and a type but content's null, and what is the consumer supposed to do with such an object?
The requirements for the public api should be as strict as possible and can be extended when necessary. We don't have a use case for having a "null" attachment and therefore should not implement it. The same for every other method from the list above. |
Are unused and the implemention in https://github.com/nextcloud/mail/pull/9651/files#diff-863bf0fae4943be717ec0db9eba3ff621d28665362e192e0b6d45a2b917426f7R190-R236 does nothing and therefore should be removed. |
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.
The API looks solid to me and I like the design overall.
However, I agree with kesselb's latest points and have some additional minor high level feedback:
- Please get rid of
IServiceIdentity
andIServiceLocation
interfaces. We can always add new interfaces once those two features are needed and ready. Currently, they seem to be unused. - All methods inside
IMessage
that return anarray
ornull
should only return an (empty)array
. This makes consuming the API more convenient as both cases don't need to be handled separately despite being semantically identical. (As kesselb mentioned, there are some more methods insideIAttachment
where this also applies.)
c3d0b64
to
f4c1801
Compare
I've changed the empty return type on the functions that return an array to an empty array, you have a valid point about the easier consumption of the array. I have also removed the extra IServiceIdentity and IServiceLocation interfaces and methods, even tho I have a feeling I might be adding them back in next month from a conversion I had this week, but I save the code so that should not be a big deal. |
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.
Aside from the comments by Daniel and Richard this looks fine.
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.
Feedback has been addressed.
91f2c96
to
cf432cf
Compare
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.
I'm aware that Beta 1 is due on 2024-07-25, but we can always backport changes if necessary.
|
|
cf432cf
to
791149f
Compare
Signed-off-by: SebastianKrupinski <krupinskis05@gmail.com>
791149f
to
fc0b694
Compare
@SebastianKrupinski this PR adds new APIs to lib/public aka OCP, so we need developer docs. You can take https://docs.nextcloud.com/server/latest/developer_manual/digging_deeper/groupware/calendar.html as inspiration. |
Documentation was added: nextcloud/documentation#12083 |
Summary
Initial Mail Provider Implementation
TODO