-
Notifications
You must be signed in to change notification settings - Fork 6
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
Add support for embedded attachments #5
base: master
Are you sure you want to change the base?
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.
Thanks for the PR!
I requested some changes. I also suggested some changes which you can apply if you agree ;)
Also, the important question I have is related to the endpoints, that I couldn't find .../attachments/item/
or .../attachments/item/...
endpoints in the Microsoft Graph documents. Would you please share the Microsoft Graph page you've followed for these endpoints?
@@ -218,6 +238,18 @@ def on_get_in_folder_by_id(self, req, resp, folderid, itemid, attachmentid=None) | |||
item = get_item_by_folder(req, folder, itemid) | |||
self._response_attachments(req, resp, item, attachmentid) | |||
|
|||
def on_get_embedded_item_in_folder_by_id(self, req, resp, folderid, itemid, attachmentid): |
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 can't find any suffix in the routes list with this name. Where does this method connect to?
self.add_route(user + '/mailFolders/{folderid}/messages/{itemid}/attachments/{attachmentid}/item', | ||
attachments, suffix="embedded_item_by_id") | ||
self.add_route(user + '/mailFolders/{folderid}/messages/{itemid}/attachments/{attachmentid}/item/$value', | ||
messages, suffix="embedded_folder_value") |
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.
Also, the same for here. Something like embedded_item_by_value
. What's your idea?
self.add_route(user + '/messages/{itemid}/attachments/{attachmentid}/item', | ||
attachments, suffix="embedded_item_by_id") | ||
self.add_route(user + '/messages/{itemid}/attachments/{attachmentid}/item/$value', | ||
messages, suffix="embedded_value") |
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.
May I ask you to change the suffix to be more consistent? Something like embedded_item_by_value
like the previous route. What's your idea?
req (Request): Falcon request object. | ||
resp (Response): Falcon response object. | ||
item (Item): instance of an item. | ||
attachmentid (str): attachment ID which is related to the item. Must be set. |
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.
attachmentid (str): attachment ID which is related to the item. Must be set. | |
attachmentid (str): attachment ID which is related to the item. |
As it doesn't have any default value, it's not needed to mention it. Because Python will raise an error anyway.
item (Item): instance of an item. | ||
attachmentid (str): attachment ID which is related to the item. Must be set. | ||
""" | ||
|
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.
if not attachmentid: | ||
raise falcon.HTTPNotFound() | ||
|
||
response = partial(self.respond, req, resp) |
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.
Just a suggestion: you can call self.respond
without using the partial
function based on your function implementation.
resp (Response): Falcon response object. | ||
folderid (str): folder ID which the item exists in. | ||
itemid (str): item ID (e.g. message ID or event ID). | ||
attachmentid (str): attachment ID which is related to the item. Must be set. |
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.
attachmentid (str): attachment ID which is related to the item. Must be set. | |
attachmentid (str): attachment ID which is related to the item. |
The same for here.
|
||
att_fields = [] | ||
for att in attachments: | ||
if att.embedded is True: |
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.
if att.embedded is True: | |
if att.embedded: |
Just a suggestion.
@@ -124,7 +126,7 @@ class MessageResource(ItemResource): | |||
deleted_resource = DeletedMessageResource | |||
|
|||
relations = { | |||
'attachments': lambda message: (message.attachments, attachment.FileAttachmentResource), # TODO embedded | |||
'attachments': lambda message: (message.attachments(embedded=True), attachment.FileAttachmentResource), # TODO embedded |
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.
All attachments are embedded?
Thanks for your comments. Just wanted to leave a note that this is not forgotten. Nevertheless, I have this code already running in a productive environment. Therefore, for testing changes, I will have to ask the admin for setting up a test environment. |
Most of the bits were there already. Encapsulating field sets per attachments in a tuple was already there to be read. Just had to be implemented in the respective function. Support for referenceAttachments should be easy to implement.
I have based that still on master as the other pull request has not been merged yet.