Skip to content
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

added ExpandDL service function for expanding public distribution lists #560

Merged
merged 6 commits into from Feb 13, 2019
Merged

Conversation

KarmaPenny
Copy link
Contributor

I implemented the ExpandDL operation from the Expand operation MSDN page

usage is similar to the non-account resolve_names method. Example:

from exchangelib import Account

a = Account(...)

# Get member mailboxes from the smtp address of a distribution list
for mailbox in a.protocol.expand_dl('distro@example.com'):
    print(mailbox.email_address)

@ecederstrand
Copy link
Owner

ecederstrand commented Feb 13, 2019

Thanks for the contribution! Very appreciated.

Apart from my review comments, this needs two lines of documentation in the README so people can search for relevant keywords and know the method exists (the Non-account methods section is probably the best place).

And finally, it needs a simple test that runs through the code path. My test server does not have distribution lists, so a test case that queries a non-existent DL and checks for ErrorNameResolutionNoResults will do.

Feel free to ask if you need help with anything :-)

@KarmaPenny
Copy link
Contributor Author

If the distribution list does not exist the API actually returns an empty list not an ErrorNameResolutionNoResults error.

From my testing this also appears to be the case for the ResolveNames operation.

Added the requested documentation to the README and created a test case for expand_dl that expands a non_existent_distro and confirms the result is an empty list.

@ecederstrand
Copy link
Owner

ecederstrand commented Feb 13, 2019

I checked out your branch, and at least on my test server, querying for a non-existent DL returns this response:

<?xml version='1.0' encoding='utf-8'?>
<s:Envelope
    xmlns:s="http://schemas.xmlsoap.org/soap/envelope/">
  <s:Header>
    <h:ServerVersionInfo
    xmlns:h="http://schemas.microsoft.com/exchange/services/2006/types" xmlns="http://schemas.microsoft.com/exchange/services/2006/types"
    xmlns:xsd="http://www.w3.org/2001/XMLSchema"
    xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" MajorVersion="15" MinorVersion="0" MajorBuildNumber="1395" MinorBuildNumber="0" Version="V2_23"/>
  </s:Header>
  <s:Body
    xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
    xmlns:xsd="http://www.w3.org/2001/XMLSchema">
    <m:ExpandDLResponse
    xmlns:m="http://schemas.microsoft.com/exchange/services/2006/messages"
    xmlns:t="http://schemas.microsoft.com/exchange/services/2006/types">
      <m:ResponseMessages>
        <m:ExpandDLResponseMessage ResponseClass="Error">
          <m:MessageText>No results were found.</m:MessageText>
          <m:ResponseCode>ErrorNameResolutionNoResults</m:ResponseCode>
          <m:DescriptiveLinkKey>0</m:DescriptiveLinkKey>
        </m:ExpandDLResponseMessage>
      </m:ResponseMessages>
    </m:ExpandDLResponse>
  </s:Body>
</s:Envelope>

I think we should treat that as an error and handle it differently than an existing list with no members.

If the server decides to return an empty list in this case, then I think that's bad behaviour, but at least we tried :-)

@KarmaPenny
Copy link
Contributor Author

Turns out I was testing with an existing list with no members. I am getting the same result as you now when testing with a non-existent address.

The code actually already throws the desired exception if that result comes back. I've updated the unit test to look for it.

@ecederstrand
Copy link
Owner

I just pulled your changes and still get an empty list for non-existing DL. You just need to remove this in ExpandDL.call():

if isinstance(elem, ErrorNameResolutionNoResults):
    continue

Also, the test can be simplified as:

    def test_expanddl(self):
        with self.assertRaises(ErrorNameResolutionNoResults):
            self.account.protocol.expand_dl('non_existent_distro@example.com')

@KarmaPenny
Copy link
Contributor Author

simplified the test case as requested.

The line you want to remove was actually copied from the ResolveNames operation. I assumed you'd want them to behave the same way.

@ecederstrand
Copy link
Owner

Ah, that's understandable, but ResolveNames takes a list of names and ExpandDL takes only one DL.

Current me thinks that ignoring the error in ResolveNames is also an error but it was made that way in a171c6f and I can't recall the exact implications now.

@KarmaPenny
Copy link
Contributor Author

looks like it was done for issue #242

Version._guess_version_from_service() needs to work even though ResolveNames.call() throws ErrorNameResolutionNoResults. Not trivial to fix, though.

We should consider letting ResolveNames ignore ErrorNameResolutionNoResults since the service returns a list anyway and could just return an empty list in this case. This is a trivial fix.

It does kinda seem like an error that ResolveNames ignores ErrorNameResolutionNoResults though. Perhaps #242 could be solved by passing a flag to ResolveNames to enable the ignore or by creating new function for guessing the version that calls the resolve name operation but ignores the error. That way actual calls to ResolveNames can produce the error when a non existent address is given.

At any rate, I've updated ExpandDL to not ignore the error.

@KarmaPenny
Copy link
Contributor Author

although I will say it is convenient to get an empty list instead of an exception considering the server normally returns a list anyways.

exchangelib/services.py Outdated Show resolved Hide resolved

:param distribution_list: SMTP address of the distribution list to expand
:return: List of Mailbox items that are members of the distribution list
"""
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be useful if this method can take either an email address as a string or a full Mailbox instance. For example, private distribution lists need an ItemId, according to the documentation of the ExpandDL service. A simple if/else should suffice:

        from .properties import Mailbox
        if isinstance(distribution_list, string_types):
            distribution_list = Mailbox(email_address=distribution_list, mailbox_type='PublicDL')

and then ExpandDL.get_payload() becomes just:

    def get_payload(self, distribution_list):
        payload = create_element('m:%s' % self.SERVICE_NAME)
        set_xml_value(payload, distribution_list, version=self.protocol.version)
        return payload

The only problem is that ExpandDL, unlike everything else, wants Mailbox to be in the 'm:' namespace in the request, so we need to create a Mailbox subclass that sets NAMESPACE = MNS and is used only for requesting DL members.

@ecederstrand ecederstrand merged commit 24d44bc into ecederstrand:master Feb 13, 2019
@ecederstrand
Copy link
Owner

Thanks for your continued efforts here :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants