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

do not log a warning for empty responses #69

Merged
merged 2 commits into from
Jan 15, 2018

Conversation

frisi
Copy link
Contributor

@frisi frisi commented Jan 11, 2018

if a text/html response does not contain any data (eg empty page for an ajax request)
we do not log a warning that no csrf token could be added

this fixes #15

not sure if the tests actually add value since i did not find a way to check no warning message is logged.

if a text/html response does not contain any data (eg empty page for an ajax request)
we do not log a warning that no csrf token could be added

this fixes #15
@frisi frisi requested a review from vangheem January 11, 2018 23:38
Copy link
Member

@mauritsvanrees mauritsvanrees left a comment

Choose a reason for hiding this comment

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

Thanks! I think this fixes issues #15 and #64.

I added one comment. Can you check that and start Jenkins jobs? The Plone tests can easily have corner cases that might trip here.

if isinstance(result, list):
# do not parse empty strings to omit warning log message
if len(result) == 1:
if result[0].strip() == u'':
Copy link
Member

Choose a reason for hiding this comment

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

I would simplify this to if not result[0].strip(): return None.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks @mauritsvanrees!
i was not sure what types of data structures can possibly be handed over to a transform's transform method.

if it is just either repoze.xmliter.serializer.XMLSerializer or a list containing one string, your simplification is fine.
if the list can possibly be empty we'd need to catch an IndexError

Copy link
Member

Choose a reason for hiding this comment

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

With my simplification I only meant that one line: instead of if X == u'' do if not X.

Copy link
Member

Choose a reason for hiding this comment

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

Note: I am not quite sure what structures can end up in here either.

@frisi
Copy link
Contributor Author

frisi commented Jan 15, 2018

thanks for your clarifications @mauritsvanrees. i simplified the if statements and started the tests

@frisi frisi removed the request for review from vangheem January 15, 2018 14:51
@frisi
Copy link
Contributor Author

frisi commented Jan 15, 2018

should be ready to merge @mauritsvanrees :-)

@mauritsvanrees mauritsvanrees merged commit ad4456d into master Jan 15, 2018
@mauritsvanrees mauritsvanrees deleted the no-warning-when-empty branch January 15, 2018 17:35
@mauritsvanrees
Copy link
Member

Merged. Thank you!

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.

ProtectTransform gives warning when trying and failing to parse response with empty body
2 participants