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

Surface errors from API calls to user on status page. #10380

Merged
merged 4 commits into from
May 20, 2017

Conversation

xurizaemon
Copy link
Member

@xurizaemon xurizaemon commented May 19, 2017

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)?


@xurizaemon
Copy link
Member Author

@colemanw do you know the status page and feel able to review this?

Copy link
Member

@colemanw colemanw left a 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 :)

'<b>Debug information:</b><br>' +
result.debug_information + '</div>';
}
CRM.alert(error_message, 'API error', 'error');
Copy link
Member

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()

Copy link
Member Author

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?).

Copy link
Member

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"?

Copy link
Member Author

@xurizaemon xurizaemon May 19, 2017

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.

image

@xurizaemon
Copy link
Member Author

xurizaemon commented May 19, 2017

See comment above for what the user sees in event of DB error on System.updateIndexes (related issue CRM-20533).

@colemanw colemanw merged commit 2e2286e into civicrm:master May 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants