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

Pathing issueIssue 2571, 2585, 2591 #943

Open
wants to merge 4 commits into
base: development
Choose a base branch
from
Open

Pathing issueIssue 2571, 2585, 2591 #943

wants to merge 4 commits into from

Conversation

Mad-Piggy
Copy link

Patch for issues:

  • Issue 2571: Plex (and possibly pushbullet) notifications no longer work
  • Issue 2585: Patch for: TypeError: coercing to Unicode: need string or buffer, instance found
  • Issue 2591: coercing to Unicode: need string or buffer, instance found

Mad-Piggy added 2 commits May 7, 2015 00:36
Patch for: TypeError: coercing to Unicode: need string or buffer,
instance found

Changed url to url.get_full_url()
There appear to be two issues.
- Plex authentication always fails (pw_mgr is not passed to getURL)
- etree.fromstring already returns the root Element. Unlike ElementTree,
Element doesn't have getroot(), nor is it needed here.

Fixes
- Add password_mgr=pw_mgr parameter to sickbeard.helpers.getURL
- remove .getroot() from media_container
@Mad-Piggy Mad-Piggy changed the title Pathing issue with Plex notifications and Coercing to unicode Pathing issueIssue 2571, 2585, 2591 May 6, 2015
if throw_exc:
raise
else:
return None

except socket.timeout:
logger.log(u"Timed out while loading URL " + url, logger.WARNING)
logger.log(u"Timed out while loading URL " + req_to_str(url),, logger.WARNING)
Copy link
Contributor

Choose a reason for hiding this comment

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

tried this pr, got syntax error here. (Double commas)

Copy link
Author

Choose a reason for hiding this comment

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

Fixed. Thx for letting me know.

there was a single comma to many.

Thx to taylorludwig for pointing it out.
@dimfeld
Copy link

dimfeld commented Jun 5, 2015

Thanks for the fix! I'm happy to have my Plex notifications working again.

@jakswa
Copy link

jakswa commented Jun 8, 2015

Thanks from me as well. Plex updates don't error anymore for me

@CyberMew
Copy link

CyberMew commented Jun 8, 2015

Is this on the Dev or master branch yet?

@dimfeld
Copy link

dimfeld commented Jun 8, 2015

I don't think so. I just manually patched it in to my local copy from the PR.

@KevinAnthony
Copy link

Is there a timeline on when this will be merged in?

@Mad-Piggy
Copy link
Author

It is indeed not yet implemented in dev or master. But this is something we have to w8 for midgetspy to do. If you realy want to pull this patch, you can from my repository: https://github.com/Mad-Piggy/Sick-Beard

@arucard21
Copy link
Contributor

Just wanted to mention that the fix for the coercion errors is also provided in #939. That problem is actually bigger than the Plex notifier but shouldn't have any impact on the functionality. It's the logging of an error that goes wrong.

I think the problem with Plex was that the password manager wasn't supplied with the getURL() call, so it failed. But due to the unrelated problem with the logging of that error, it wasn't clear what exactly was going wrong. So perhaps it would be easier to have this merged if both PR's focus on fixing only 1 thing, with #939 for the logging fix and this PR for the Plex fix. I think including too many things in 1 PR might make it harder to determine if it can (and should) be merged (but I could be wrong about that).

Note that with only the plex fix your branch would no longer cause an error when connecting to plex and the error would no longer need to be logged so the coercion problem wouldn't occur anymore either. Just to say that it would still remain entirely functional.

@jordansaints
Copy link

Just wanted to say thanks for the fixes! Saved them in a stable branch and am never updating SB again.

@jpanski
Copy link

jpanski commented Oct 10, 2015

Any eta on when this pull request will be merged into development or master?

@dimfeld
Copy link

dimfeld commented Oct 10, 2015

I think development on SickBeard has ceased... Personally I've switched to SickRage which is somewhat based on this one but still under active development.

@jpanski
Copy link

jpanski commented Oct 10, 2015

Wow, that's really sad if true. I hope that's not the case and he's just on a sabbatical.

@dimfeld
Copy link

dimfeld commented Oct 10, 2015

Yeah, hard to tell. He hasn't said anything on the forums at sickbeard.com either, so nobody really knows for sure...

@Amadeus-
Copy link

By the way, if folks want to apply these patches (and/or any others while the primary developer is missing), there are good instructions here: http://stackoverflow.com/a/28729484. Basically, you can create pull requests between the requests here and your own forked repository.

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.

10 participants