Skip to content

convert user objects to unicode, improve error logging - #424

Merged
adampalay merged 2 commits into
masterfrom
fix/adam/dash-reset
Aug 1, 2013
Merged

convert user objects to unicode, improve error logging#424
adampalay merged 2 commits into
masterfrom
fix/adam/dash-reset

Conversation

@adampalay

Copy link
Copy Markdown
Contributor

There were a couple of user objects that weren't being converted to unicode strings in instructor/views.

This was causing a bug where student answers appeared not to be reset.

@ichuang
@brianhw

addresses https://edx-wiki.atlassian.net/browse/LMS-693

Comment thread lms/djangoapps/instructor/views.py Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd like to see the error message appearing in the msg as well. I think the rescore code below does that.

@adampalay

Copy link
Copy Markdown
Contributor Author

@brianhw , @nedbat can you give this the thumbs up?

Comment thread lms/djangoapps/instructor/views.py Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we really have anything to report here? Not clear that the error message is different (or nicer) than the original one.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Usually, you get the exception into a string with just: "Oh noes: {0}".format(err)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The idea was that we'd just get more information to understand what went wrong

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

For future debugging

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

But here nothing particularly went wrong, except user error. They typed in a urlname value that didn't find an existing module. It's not something that we should be logging, I don't think, and certainly not as an exception!

@adampalay

Copy link
Copy Markdown
Contributor Author

Ok, I'm seeing two criticisms here:

  1. We shouldn't be logging insignificant exception errors
  2. We shouldn't be logging errors at html
  3. We shouldn't be displaying our errors.

2 totally makes sense. That's my bad. My question is should we be logging any of these exceptions? Should we be displaying any of these errors to the instructor?

Comment thread lms/djangoapps/instructor/views.py Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I still don't think we should be logging this exception. We know it's a DoesNotExist error, so logging the exception won't provide information, and it will create a lot of noise.

@brianhw

brianhw commented Jul 30, 2013

Copy link
Copy Markdown
Contributor

The original motivation was to see what the errors where when staff are trying to reset attempts. It was failing, but we didn't know why. So having that be reported, both on screen and in log files, would be useful. Same goes for the delete case. But not for the not-a-recognized-problem case.

@adampalay

Copy link
Copy Markdown
Contributor Author

That makes a lot of sense.

@adampalay

Copy link
Copy Markdown
Contributor Author

Does this look better?

Comment thread lms/djangoapps/instructor/views.py Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What you have could be written more succinctly as msg += current_msg. I would prefer to have the error message appear in msg and not in the string passed to log.exception, so what I said before about message reuse was wrong.

@brianhw

brianhw commented Aug 1, 2013

Copy link
Copy Markdown
Contributor

Looks good. Just squash the commits and go ahead and merge.

adampalay added a commit that referenced this pull request Aug 1, 2013
convert user objects to unicode, improve error logging
@adampalay
adampalay merged commit 76cf202 into master Aug 1, 2013
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.

3 participants