Handle invalid issues (#18111)

* Handle invalid issues

- When you hover over a issue reference, and the issue doesn't exist, it
will just hang on the loading animation.
- This patch fixes that by showing them the pop-up with a "Error
occured" message.

* Add I18N

* refactor

* fix comment for lint

* fix unit test for i18n

* fix unit test for i18n

* add comments

Co-authored-by: Lunny Xiao <[email protected]>
Co-authored-by: wxiaoguang <[email protected]>

Author: 3 people

integrations/api_releases_test.go

@@ -207,7 +207,7 @@ func TestAPIGetReleaseByTag(t *testing.T) {
 
 	var err *api.APIError
 	DecodeJSON(t, resp, &err)
-	assert.EqualValues(t, "Not Found", err.Message)
+	assert.NotEmpty(t, err.Message)
 }
 
 func TestAPIDeleteReleaseByTagName(t *testing.T) {

integrations/repo_branch_test.go

@@ -141,7 +141,7 @@ func TestCreateBranchInvalidCSRF(t *testing.T) {
 	resp = session.MakeRequest(t, NewRequest(t, "GET", loc), http.StatusOK)
 	htmlDoc := NewHTMLParser(t, resp.Body)
 	assert.Equal(t,
-		"Bad Request: Invalid CSRF token",
+		"Bad Request: invalid CSRF token",
 		strings.TrimSpace(htmlDoc.doc.Find(".ui.message").Text()),
 	)
 }

modules/context/api.go

@@ -30,6 +30,11 @@ type APIContext struct {
 	Org *APIOrganization
 }
 
+// Currently, we have the following common fields in error response:
+// * message: the message for end users (it shouldn't be used for error type detection)
+//            if we need to indicate some errors, we should introduce some new fields like ErrorCode or ErrorType
+// * url:     the swagger document URL
+
 // APIError is error format response
 // swagger:response error
 type APIError struct {
@@ -47,8 +52,8 @@ type APIValidationError struct {
 // APIInvalidTopicsError is error format response to invalid topics
 // swagger:response invalidTopicsError
 type APIInvalidTopicsError struct {
-	Topics  []string `json:"invalidTopics"`
-	Message string   `json:"message"`
+	Message       string   `json:"message"`
+	InvalidTopics []string `json:"invalidTopics"`
 }
 
 //APIEmpty is an empty response
@@ -122,9 +127,9 @@ func (ctx *APIContext) InternalServerError(err error) {
 	})
 }
 
-var (
-	apiContextKey interface{} = "default_api_context"
-)
+type apiContextKeyType struct{}
+
+var apiContextKey = apiContextKeyType{}
 
 // WithAPIContext set up api context in request
 func WithAPIContext(req *http.Request, ctx *APIContext) *http.Request {
@@ -351,7 +356,7 @@ func ReferencesGitRepo(allowEmpty bool) func(http.Handler) http.Handler {
 // NotFound handles 404s for APIContext
 // String will replace message, errors will be added to a slice
 func (ctx *APIContext) NotFound(objs ...interface{}) {
-	var message = "Not Found"
+	var message = ctx.Tr("error.not_found")
 	var errors []string
 	for _, obj := range objs {
 		// Ignore nil
@@ -367,9 +372,9 @@ func (ctx *APIContext) NotFound(objs ...interface{}) {
 	}
 
 	ctx.JSON(http.StatusNotFound, map[string]interface{}{
-		"message":           message,
-		"documentation_url": setting.API.SwaggerURL,
-		"errors":            errors,
+		"message": message,
+		"url":     setting.API.SwaggerURL,
+		"errors":  errors,
 	})
 }
 

options/locale/locale_en-US.ini

@@ -104,10 +104,12 @@ error404 = The page you are trying to reach either <strong>does not exist</stron
 never = Never
 
 [error]
-occurred = An error has occurred
-report_message = If you are sure this is a Gitea bug, please search for issue on <a href="https://github.com/go-gitea/gitea/issues">GitHub</a> and open new issue if necessary.
+occurred = An error occurred
+report_message = If you are sure this is a Gitea bug, please search for issues on <a href="https://github.com/go-gitea/gitea/issues" target="_blank">GitHub</a> or open a new issue if necessary.
 missing_csrf = Bad Request: no CSRF token present
-invalid_csrf = Bad Request: Invalid CSRF token
+invalid_csrf = Bad Request: invalid CSRF token
+not_found = The target couldn't be found.
+network_error = Network error
 
 [startpage]
 app_desc = A painless, self-hosted Git service

templates/base/head.tmpl

@@ -46,10 +46,13 @@
 			]).values()),
 			{{end}}
 			mermaidMaxSourceCharacters: {{MermaidMaxSourceCharacters}},
+			{{/* this global i18n object should only contain gereral texts. for specalized texts, it should be provied inside the related modules by: (1) API response (2) HTML data-attribute (3) PageData */}}
 			i18n: {
 				copy_success: '{{.i18n.Tr "copy_success"}}',
 				copy_error: '{{.i18n.Tr "copy_error"}}',
-			}
+				error_occurred: '{{.i18n.Tr "error.occurred"}}',
+				network_error: '{{.i18n.Tr "error.network_error"}}',
+			},
 		};
 		{{/* in case some pages don't render the pageData, we make sure it is an object to prevent null access */}}
 		window.config.pageData = window.config.pageData || {};

web_src/js/components/ContextPopup.vue

@@ -16,13 +16,17 @@
         </div>
       </div>
     </div>
+    <div v-if="!loading && issue === null">
+      <p><small>{{ i18nErrorOccurred }}</small></p>
+      <p>{{ i18nErrorMessage }}</p>
+    </div>
   </div>
 </template>
 
 <script>
 import {SvgIcon} from '../svg.js';
 
-const {appSubUrl} = window.config;
+const {appSubUrl, i18n} = window.config;
 
 // NOTE: see models/issue_label.go for similar implementation
 const srgbToLinear = (color) => {
@@ -49,7 +53,9 @@ export default {
 
   data: () => ({
     loading: false,
-    issue: null
+    issue: null,
+    i18nErrorOccurred: i18n.error_occurred,
+    i18nErrorMessage: null,
   }),
 
   computed: {
@@ -112,14 +118,20 @@ export default {
   methods: {
     load(data, callback) {
       this.loading = true;
-      $.get(`${appSubUrl}/api/v1/repos/${data.owner}/${data.repo}/issues/${data.index}`, (issue) => {
+      this.i18nErrorMessage = null;
+      $.get(`${appSubUrl}/api/v1/repos/${data.owner}/${data.repo}/issues/${data.index}`).done((issue) => {
         this.issue = issue;
+      }).fail((jqXHR) => {
+        if (jqXHR.responseJSON && jqXHR.responseJSON.message) {
+          this.i18nErrorMessage = jqXHR.responseJSON.message;
+        } else {
+          this.i18nErrorMessage = i18n.network_error;
+        }
+      }).always(() => {
         this.loading = false;
-        this.$nextTick(() => {
-          if (callback) {
-            callback();
-          }
-        });
+        if (callback) {
+          this.$nextTick(callback);
+        }
       });
     }
   }