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.

Sign up for GitHub

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

Jump to bottom

Handle invalid issues #18111

Merged
merged 8 commits into from
Dec 28, 2021
Merged

Handle invalid issues #18111

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
202c3aa
Handle invalid issues
Dec 26, 2021
3b87a0d
Add I18N
Dec 26, 2021
cc7658c
Merge branch 'main' into handle-invalid-issues
lunny Dec 27, 2021
01f2dfc
refactor
wxiaoguang Dec 28, 2021
b177198
fix comment for lint
wxiaoguang Dec 28, 2021
d95ccf1
fix unit test for i18n
wxiaoguang Dec 28, 2021
663b97b
fix unit test for i18n
wxiaoguang Dec 28, 2021
b14f115
add comments
wxiaoguang Dec 28, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 14 additions & 9 deletions modules/context/api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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{}) {
Copy link
Member

Choose a reason for hiding this comment

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

Would be nice to pass here i18n error that issue/PR was not found

Copy link
Contributor

Choose a reason for hiding this comment

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

The reason that I didn't introduce a new i18n translation is: I think the default "not_found" message is clear enough to most users, and we can keep our system small and simple without introducing new texts (and it reduces maintenance work in future).

I am fine with using either a specialized i18n text (new) or a general i18n text (as now).

Not sure how you think about this: either is fine? or should we introduce specialized i18n texts?

var message = "Not Found"
var message = ctx.Tr("error.not_found")
var errors []string
for _, obj := range objs {
// Ignore nil
Expand All @@ -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,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could this be seen as a breaking change? As it's changing a JSON field.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, it changes the field name (make the field name same as other API response). IMO no client could use this field to do anything useful, since it is just a URL for a API document, so it should be fine.

"errors": errors,
})
}

Expand Down
8 changes: 5 additions & 3 deletions options/locale/locale_en-US.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 3 additions & 1 deletion templates/base/head.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,9 @@
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 || {};
Expand Down
28 changes: 20 additions & 8 deletions web_src/js/components/ContextPopup.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) => {
Expand All @@ -49,7 +53,9 @@ export default {
data: () => ({
loading: false,
issue: null
issue: null,
i18nErrorOccurred: i18n.error_occurred,
i18nErrorMessage: null,
}),
computed: {
Expand Down Expand Up @@ -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);
}
});
}
}
Expand Down