-
-
Notifications
You must be signed in to change notification settings - Fork 825
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
Surface errors from API calls to user on status page. #10380
Surface errors from API calls to user on status page. #10380
Conversation
@colemanw do you know the status page and feel able to review this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks perfectly fine to me. Nice first patch in Angular :)
ang/crmStatusPage/StatusPageCtrl.js
Outdated
'<b>Debug information:</b><br>' + | ||
result.debug_information + '</div>'; | ||
} | ||
CRM.alert(error_message, 'API error', 'error'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrap 'API error' in ts()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. Have done that for a couple other strings also (should be harmless, even if "DB Error: already exists" doesn't exist in ts dicts yet?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@xurizaemon Actually I don't think "API error" is very descriptive, since the api is just a gateway to, well everything in the system. Maybe "Operation Failed"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good thinking. And if I change the signature of refresh()
to include a title, then I can say which operation failed. Updated (lmk if that signature change seems a problem, note this includes a change to the "hush" actions which use refresh()
to set a statuspref.
This change assumes that the list of actions to refresh()
has a single title (array of actions, string title). Is there a case where this page triggers multiple actions in a single shot? The API calls passed don't each have a friendly title, but they could if we need.
See comment above for what the user sees in event of DB error on |
CRM-20602
This is pretty my-first-angular, so please be firm re where things like .status-debug-information really belong.
If it sees "debug_information" it appends it to the alert. That could get ugly but it's better than nothing; an alternative is to direct people to the debug log (or, make it available but hidden, or both)?