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

Feature: Add milestones search by name on dashboard milestones page. #14806

Closed
wants to merge 5 commits into from
Closed

Conversation

rogerluo410
Copy link
Contributor

Feature for issue 13845:
Add milestones search by name on dashboard milestones page.

@6543 6543 added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Feb 27, 2021
<form class="ui form ignore-dirty">
<div class="ui search fluid action input">
<input type="hidden" name="type" value="{{$.ViewType}}"/>
<input type="hidden" name="repos" value="[{{range $.RepoIDs}}{{.}}%2C{{end}}]"/>
Copy link
Member

Choose a reason for hiding this comment

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

Somehow this value gets URL-encoded, which is not understood by gitea.
Maybe the backend should URI-decode properly, instead of fixing this on the frontend

Copy link
Contributor Author

@rogerluo410 rogerluo410 Mar 1, 2021

Choose a reason for hiding this comment

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

Hi, noerw, I didn't make sense of your suggestion. The %2C translates to a comma (,). What you mean is we should not use the encoding '%2c' in the URL query params on the frontend ?

I just reference it on the issue.tmpl (FYI:

<input type="hidden" name="repos" value="[{{range $.RepoIDs}}{{.}}%2C{{end}}]"/>
), there is the same way to generate the query params. And the backend uses the regular express var issueReposQueryPattern = regexp.MustCompile(^\[\d+(,\d+)*,?\]$) to fetch them from URL query params.

Maybe we should use a unified way to solve it in the whole project?

Copy link
Member

Choose a reason for hiding this comment

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

The issue is the double-escaped comma, the form handler that extracts the value from the hidden input already url-escapes.
To fix this:

Suggested change
<input type="hidden" name="repos" value="[{{range $.RepoIDs}}{{.}}%2C{{end}}]"/>
<input type="hidden" name="repos" value="[{{range $.RepoIDs}}{{.}},{{end}}]"/>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

replaced.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 28, 2021
@noerw noerw added this to the 1.15.0 milestone Mar 1, 2021
Copy link
Member

@noerw noerw left a comment

Choose a reason for hiding this comment

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

Looks good, but it would be nice to add this to the <repo>/milestones page as well.

@rogerluo410
Copy link
Contributor Author

@noerw I created new PR to implement both features (search on dashboard / repo), It may close the PR after merging that PR. FYI: #14866

@noerw noerw closed this Mar 3, 2021
@6543 6543 removed this from the 1.15.0 milestone Mar 3, 2021
@6543 6543 removed type/feature Completely new functionality. Can only be merged if feature freeze is not active. lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Mar 3, 2021
@go-gitea go-gitea locked and limited conversation to collaborators May 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants