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

Made support-email configurable, fixes #100 #108

Merged
merged 5 commits into from
Jan 19, 2016

Conversation

sedrubal
Copy link
Member

  • needs testing, because it would be bad if no error message will be displayed, because of such a trivial mistake, but I don't know how to trigger suitable errors, besides the DebtLimitExceeded exception - there it works...
  • is vulnerable to crash on missing config option #63

@landscape-bot
Copy link

Code Health
Repository health decreased by 0.14% when pulling 7c528af on support-mail-config into 4a4d884 on development.

@landscape-bot
Copy link

Code Health
Repository health decreased by 0.06% when pulling 9d1b4de on support-mail-config into 4a4d884 on development.

@sedrubal sedrubal added the wip label Nov 22, 2015
@patkan patkan assigned sedrubal and unassigned patkan Nov 23, 2015
@patkan
Copy link
Member

patkan commented Nov 23, 2015

@sedrubal I was just too fast with assigning, the commit should fix your problem ;-)

@patkan
Copy link
Member

patkan commented Nov 27, 2015

Seems to be ok.
@sedrubal you could rebase this PR to one commit and then we could merge it.

@@ -64,11 +64,12 @@ def myNewExceptionHook(exctype, value, tb):
import datetime
# logging.exception()
try:
cfg = getConfig()
email = cfg.get('general', 'support_mail')
Copy link
Member

Choose a reason for hiding this comment

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

please catch ConfigParser.Error here so that the crash handler can't crash

@patkan patkan force-pushed the support-mail-config branch from ebd26a9 to b14f1ad Compare November 27, 2015 16:39
@patkan
Copy link
Member

patkan commented Nov 27, 2015

Sorry, had to rebase due to sloppiness.
This should work now.

email = cfg.get('general', 'support_mail')
except ConfigParserError:
logging.warning("could not read mail address from config in graphical except-hook.")
email = "dex Verantwortlichx"
Copy link
Member

Choose a reason for hiding this comment

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

typo? or gender bullshit?

Copy link
Member

Choose a reason for hiding this comment

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

Man muss natürlich richtig gendern und so.
Spaß beiseite, ich dachte mir man kann das als easter egg einbauen, du darfst es aber auch gerne ändern :-P

sedrubal and others added 2 commits January 16, 2016 09:24
 - needs testing,
 - is vulnerable to #63

This commit contains

9d1b4de
Fixed syntax, TODO: cfg

and

0ffd19a
FIX pass cfg object into payment_methods
@patkan patkan force-pushed the support-mail-config branch from 1e36da0 to 5f8bbe5 Compare January 16, 2016 08:28
@patkan
Copy link
Member

patkan commented Jan 16, 2016

I have rebased this branch against current development.

@sedrubal
Copy link
Member Author

Can we merge this? :)

@mgmax
Copy link
Member

mgmax commented Jan 16, 2016

the 'dex Verantwortlichx' should still be changed, maybe just to 'den Verantwortlichen'.

@sedrubal
Copy link
Member Author

I like this hidden ironical political and social criticism. But if you worry about that, I think its no problem to change this.

Sometime in the future we should also change such messages to english and use internationalization and translations...

@patkan patkan force-pushed the support-mail-config branch from 1943beb to e6ec98d Compare January 17, 2016 11:02
@patkan
Copy link
Member

patkan commented Jan 17, 2016

we could try to implement a test, that checks whether the graphical except-hook is working. However, in sum this is probably not worth the time.

@sedrubal
Copy link
Member Author

I think, we can merge this, too. Until 21:00

@patkan
Copy link
Member

patkan commented Jan 19, 2016

That's OK for me.

@sedrubal sedrubal merged commit e6ec98d into development Jan 19, 2016
sedrubal pushed a commit that referenced this pull request Jan 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants