give some debug message regarding why export has failed #589

chrisndodge · 2013-08-06T03:08:57Z

To test:

create a problem
muck with the XML to make it invalid
go export it
should see error notification with some debugging information

@talbs can you look from a UI side. I reused existing components (I hope). @dmitchell can you review back-end code? Not sure how to unit test since this is a hook into the UI.

talbs · 2013-08-06T13:18:26Z

@chrisndodge, thanks for trying to make the status communication better in this view. I've taking a spin through and created an error (screengrab - http://cl.ly/image/411U0f3o3y11).

I'm not sure using the "alert" style UI is the best way to show this error. Perhaps a prompt UI might be better - to force a user to acknowledge that an error occurred and to have them take direct action (by clicking a button to confirm/return to try again).

Also, is the technical error output what we want to display? Can we make this more into a summary sentence with more importantly a link to the Unit where the error happened (for easy correction)?

@frrrances or @marcotuts, what do you guys think of the best UI shape to communicate this error?

talbs · 2013-08-06T13:22:16Z

cms/templates/export.html

+          intent: "error",
+          closeIcon: false
+      });
+      dialog.show();


@chrisndodge, I'm not sure we're using this method of showing alerts elsewhere (our plan on the design side was to have CSS-based classes control the visibility and animation of elements rather than relying on JQuery's .show()/.hide() methods.

It looks like you may be adding this to make the alert UI accommodate a case it was never supposed to - asynchronous error messaging (it was meant to be displayed when a page was loaded after a traditional page request).

Can we get Peter Fogg or DB or Julian to wire up the notification mechanism
on the client instead of pushing it on Chris?

On Tue, Aug 6, 2013 at 9:22 AM, Brian Talbot notifications@github.comwrote:

In cms/templates/export.html:

@@ -6,6 +6,24 @@
<%block name="title">${_("Course Export")}</%block>
<%block name="bodyclass">is-signedin course tools export</%block>

+<%block name="jsextra">

% if err_msg:

<script type='text/javascript'>

$(document).ready(function() {

var defaultTitle = gettext('There has been an error with your export.');

var msg = "${err_msg}";

dialog = new CMS.Views.Alert.Confirmation({

title: defaultTitle,

message: msg,

intent: "error",

closeIcon: false

});

dialog.show();

@chrisndodge https://github.com/chrisndodge, I'm not sure we're using
this method of showing alerts elsewhere (our plan on the design side was to
have CSS-based classes control the visibility and animation of elements
rather than relying on JQuery's .show()/.hide() methods.

It looks like you may be adding this to make the alert UI accommodate a
case it was never supposed to - asynchronous error messaging (it was meant
to be displayed when a page was loaded after a traditional page request).

—
Reply to this email directly or view it on GitHubhttps://github.com/edx/edx-platform/pull/589/files#r5603493
.

I believe that @chrisndodge is doing the right thing here. The .show() and .hide() methods on a notification view don't proxy to jQuery's show/hide JS animations, they simply inform the view whether it should be shown or hidden, and ask the view to re-render itself: code is here. The actual animation in the view is handled by the CSS classes that are being applied to the rendered HTML.

talbs · 2013-08-06T13:24:05Z

Also, @chrisndodge, is there a bug or sprint story associated with this? I want to make sure I'm reviewing based on any requirements/specifics detailed elsewhere.

@talbs Looks like this is STUD-568. https://edx-wiki.atlassian.net/browse/STUD-568 (Christina)

dmitchell · 2013-08-06T13:31:02Z

cms/djangoapps/contentstore/views/assets.py

@@ -335,16 +336,34 @@ def generate_export_course(request, org, course, name):
    the course
    """
    location = get_location_and_verify_access(request, org, course, name)
-
+    course_module = modulestore().get_item(location)


I thought you were deprecating get_item and moving to get_instance w/ the course_id as first arg

Line still says get_item(location) rather than get_instance(location.course_id, location). I don't really care, but it seems that if we're trying to deprecate get_item, we shouldn't add uses of it.

Sorry, I changed a different line from get_item -> get_instance. Doing this one too.

singingwolfboy · 2013-08-07T18:44:50Z

Avoid constructing HTML in a view function, even just   tags -- leave that to the template. I suggest that you just pass the failed_item to the template, and let the template construct the error message using that object.

chrisndodge · 2013-08-08T02:26:45Z

@singingwolfboy @dmitchell @talbs , thanks for all the great feedback. I did another pass tonight using the "prompt" notification and I present the user with the action to be able to go to the Edit Unit page to fix the module in error. If the error is not happening at a component level (inside a Unit), then the messaging is a bit more generic (including the raw exception message) and the user can click to go to the course overview page.

Hope this addresses most of the feedback.

singingwolfboy · 2013-08-08T19:46:07Z

cms/templates/export.html

+            }
+        });
+      % else:
+        var msg = gettext("<p>There has been a failure to export your course to XML. Unfortunately, we do not have specific enough information to assist you in identifying the failed component. It is recommended that you inspect your courseware to identify any components in error and try again.</p><p>The raw error message is:</p>");


Don't translate HTML. You can do:

var msg = "" + gettext("There has been a failure...") + "" + gettext("The raw error message is:") + "";

singingwolfboy · 2013-08-08T19:49:47Z

Looks pretty good! I just identified a few small issues; the rest looks good to me.

chrisndodge · 2013-08-08T20:04:35Z

@singingwolfboy thanks for the feedback, updated PR with your comments.

singingwolfboy · 2013-08-08T20:15:20Z

👍

chrisndodge · 2013-08-09T00:37:22Z

@dmitchell did change from get_item -> get_instance

…o the edit unit page which contains the module in error. Otherwise, present the raw exception message and allow user to go to the course outline page.

chrisndodge · 2013-08-09T01:25:12Z

Waiting for @talbs visual review, but he's having localdev troubles. Will work with him on Friday AM to try to close this out.

talbs · 2013-08-09T14:01:53Z

👍 here - I just tweaked the copy of the two controls at the bottom of the prompt.

…ging give some debug message regarding why export has failed

…dback-update-3 Hash Update #3: Diagnostic-Feedback

* Proversity/development (openedx#558) ENH: bulk grades api to be granular ENH: course order by ADD: harambee custom backend SSO FIX: show correct course info on instructor dashboard FIX: course re-run FIX: course date settings in studio. section release dates are no reflected and updated from the ADD: missing welsh translations FIX: invalid gettext call for translating js

* Proversity/development (#558) ENH: bulk grades api to be granular ENH: course order by ADD: harambee custom backend SSO FIX: show correct course info on instructor dashboard FIX: course re-run FIX: course date settings in studio. section release dates are no reflected and updated from the ADD: missing welsh translations FIX: invalid gettext call for translating js

BC-19_P3 opchore: Test xblocks supported for lilac/limonero.

Co-authored-by: Renovate Bot <bot@renovateapp.com>

talbs reviewed Aug 6, 2013
View reviewed changes

dmitchell reviewed Aug 6, 2013
View reviewed changes

singingwolfboy reviewed Aug 8, 2013
View reviewed changes

Chris Dodge added 5 commits August 8, 2013 21:01

give some debug message regarding why export might fail

fcaf3e6

switch the notification to be a prompt and allow for the user to go t…

69c34a6

…o the edit unit page which contains the module in error. Otherwise, present the raw exception message and allow user to go to the course outline page.

handle exceptions inside the outer exception handling

4054544

implement PR feedback

487ae96

get_item -> get_instance

32d92d9

Studio: revises export prompt control copy

5b4c15a

chrisndodge pushed a commit that referenced this pull request Aug 9, 2013

Merge pull request #589 from edx/fix/cdodge/better-export-error-messa…

09a83dc

…ging give some debug message regarding why export has failed

chrisndodge merged commit 09a83dc into master Aug 9, 2013

itsjeyd referenced this pull request in open-craft/edx-platform Dec 23, 2015

Merge pull request #589 from edx-solutions/jareerahsan/diagnostic-fee…

99312bc

…dback-update-3 Hash Update #3: Diagnostic-Feedback

mariajgrimaldi pushed a commit to eduNEXT/edx-platform that referenced this pull request Dec 17, 2021

Merge pull request openedx#589 from eduNEXT/li/ednx/BC-19_P3

1f379c3

BC-19_P3 opchore: Test xblocks supported for lilac/limonero.

baby636 mentioned this pull request May 14, 2022

[Snyk] Fix for 1 vulnerabilities baby636/edx-platform#22

Open

Sujeet1379 pushed a commit to chandrudev/edx-platform that referenced this pull request Nov 17, 2022

chore(deps): update codecov/codecov-action action to v2 (openedx#589)

0394118

Co-authored-by: Renovate Bot <bot@renovateapp.com>

baby636 mentioned this pull request Nov 28, 2023

[Snyk] Fix for 37 vulnerabilities baby636/edx-platform#54

Open

sumodgeorge mentioned this pull request Nov 28, 2023

[Snyk] Fix for 6 vulnerabilities sumodgeorge/edx-platform#15

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

give some debug message regarding why export has failed #589

give some debug message regarding why export has failed #589

chrisndodge commented Aug 6, 2013

talbs commented Aug 6, 2013

talbs Aug 6, 2013

dmitchell Aug 6, 2013

singingwolfboy Aug 7, 2013

talbs commented Aug 6, 2013

dmitchell Aug 6, 2013

chrisndodge Aug 8, 2013

dmitchell Aug 8, 2013

chrisndodge Aug 9, 2013

singingwolfboy commented Aug 7, 2013

chrisndodge commented Aug 8, 2013

singingwolfboy Aug 8, 2013

singingwolfboy commented Aug 8, 2013

chrisndodge commented Aug 8, 2013

singingwolfboy commented Aug 8, 2013

chrisndodge commented Aug 9, 2013

chrisndodge commented Aug 9, 2013

talbs commented Aug 9, 2013

give some debug message regarding why export has failed #589

give some debug message regarding why export has failed #589

Conversation

chrisndodge commented Aug 6, 2013

talbs commented Aug 6, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

talbs commented Aug 6, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

singingwolfboy commented Aug 7, 2013

chrisndodge commented Aug 8, 2013

Choose a reason for hiding this comment

singingwolfboy commented Aug 8, 2013

chrisndodge commented Aug 8, 2013

singingwolfboy commented Aug 8, 2013

chrisndodge commented Aug 9, 2013

chrisndodge commented Aug 9, 2013

talbs commented Aug 9, 2013