mrkipling / maraschino

A front-end for HTPCs
MIT License
424 stars 125 forks source link

transmission module: fixed rpc url config parameter for custom webroot #330

Closed vajonam closed 10 years ago

vajonam commented 11 years ago

add rpc url as configurable parameter for transmission module.

vajonam commented 11 years ago

thanks, i have i found the root of the problem.

it appears that

in the function get_settings_value

    #Strip http/https from hostnames
    if key.endswith('_host') or key.endswith('_ip'):
        if value.startswith('http://'):
            return value[7:]
        elif value.startswith('https://'):
            return value[8:]
    return value

which means the URL is being stripped of the HTTP this breaks the lib/transmission/client.py where it expects a url scheme in address.

       def __init__(self, address='localhost', port=DEFAULT_PORT, user=None, password=None, http_handler=None, timeout=None):
            if isinstance(timeout, (int, long, float)):
                self._query_timeout = float(timeout)
            else:
                self._query_timeout = DEFAULT_TIMEOUT
            urlo = urlparse.urlparse(address)
            if urlo.scheme == '':
                base_url = 'http://' + address + ':' + str(port)
                self.url = base_url + '/transmission/rpc'
            else:
                if urlo.port:
                    self.url = urlo.scheme + '://' + urlo.hostname + ':' + str(urlo.port) + urlo.path
                else:
                    self.url = urlo.scheme + '://' + urlo.hostname + urlo.path
                LOGGER.info('Using custom URL "' + self.url + '".')

so really what we need is a get_settings_value without a strip for transmission.

I have updated the pull request for this.

vajonam commented 10 years ago

is there a problem with this PR? any reason why it hasn't been pulled?

N3MIS15 commented 10 years ago

I'm not sure what the problem is. I have tried transmission module with ip address and http:// + ipaddress and both work. what exactly is this fixing (how can the issue be replicated?)

vajonam commented 10 years ago

To Replicate:

Change transmission webroot to something other than "/transmission", I have changed mine to "/bt" so the url to reach transmission is

http://dvr:9091/bt/rpc

This is what I need to set so that we can still get status from transmission.

The problem is that when we save it we strip out the http or https which breaks then transmision client lib that we are using.

N3MIS15 commented 10 years ago

OK. I now see how your fix tricks the client library to work. IMO this is not the prefered way to fix the issue. I think some parsing should be done in modules/transmission.py to make sure we send the right data to the client lib. AFAIK the "app_link" function also does not parse urls correctly in this module. Let me see if I can come up with a more robust fix.

vajonam commented 10 years ago

IMO the saving should not strip out stuff in general. Why are we removing information and trying to rebuild it later, instead we can give all the info to the various modules and have them work it.

N3MIS15 commented 10 years ago

the thing I'm worried about is if someone enters a url like this: 192.168.1.11:9091/bt/rpc

Because the http is not there the client lib will handle it a different way and just add "/transmission/rpc" to the end of it.

Thats why I think we need to parse it first.

vajonam commented 10 years ago

other bits of software usually allow the storing of a "webroot" as well, for example the sabnzbd settings includes "Webroot" so maybe just adding that to transmission fixes it..too

N3MIS15 commented 10 years ago

i have no idea if transmission supports https or not but many other apps do and we need to know whether or not we are parsing a secure url or not.

Adding webroot as an option in transmission module is exactly what I am doing.

vajonam commented 10 years ago

Cool. That works, I was trying to keep settings/parameter creep down, and was using the existing param but to your other point that if folks didn't provide the http(s) that would break it

N3MIS15 commented 10 years ago

@vajonam can you please test with this branch: https://github.com/N3MIS15/maraschino/tree/trans_webroot if all is well I will pull it to master.

vajonam commented 10 years ago

works fine.

N3MIS15 commented 10 years ago

merged #353 so closing this. Thanks for your patience @vajonam