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

Implemented filtering by repository on my tickets page #259

Merged
merged 9 commits into from May 27, 2015

Conversation

Projects
None yet
4 participants
@jeyoung
Contributor

jeyoung commented May 19, 2015

OK, I think I got it right now. Pulled develop, then merged my ticket-57 changes on top of it.

@lygstate

This comment has been minimized.

Should replace("\r", "\n")

This comment has been minimized.

Owner

gitblit replied Apr 16, 2015

Maybe. Where would that happen?

This comment has been minimized.

lygstate replied Apr 18, 2015

MacOS?

@jeyoung jeyoung changed the title from Ticket 57 develop to Implemented filtering by repository on my tickets page May 19, 2015

// by repository
List<RepositoryModel> repositoryChoices = getRepositoryModels();
RepositoryModel noneChoice = new RepositoryModel();
noneChoice.name = Translation.get("gb.all");

This comment has been minimized.

@gitblit

gitblit May 22, 2015

Owner

We can't use Transation. Instead do this:

getString("gb.all");
@@ -278,6 +320,12 @@ public void populateItem(final Item<TicketSort> item) {
}
}
if (!qb.containsField(Lucene.repository.name()) && !StringUtils.isEmpty(repository)) {

This comment has been minimized.

@gitblit

gitblit May 22, 2015

Owner

Resetting back to all doesn't work right.

If you add something like this it will:

        if (noneChoice != currentRepository && !qb.containsField(Lucene.repository.name()) ...
@@ -74,6 +76,7 @@ public MyTicketsPage(PageParameters params) {
final String queryParam = (params == null || StringUtils.isEmpty(params.getString("q", null))) ? "watchedby:" + username : params.getString("q", null);
final String searchParam = (params == null) ? "" : params.getString("s", null);
final String sortBy = (params == null) ? "" : Lucene.fromString(params.getString("sort", Lucene.created.name())).name();
final String repository = (params == null) ? "" : params.getString("repository", null);

This comment has been minimized.

@gitblit

gitblit May 22, 2015

Owner

Should use Lucene.repository.name() for consistency.

@gitblit

This comment has been minimized.

Owner

gitblit commented May 22, 2015

Aside from the little comments I made we have two larger problems. One is easily fixable. The other.... not sure.

  1. You can't search by the repository name. It's problematic because of punctuation marks, slashes, etc. Instead we must specify and search for the rid which is a hash of the repository name. So everywhere you specify a repository name value it must be the rid instead. RepositoryModel.getRID() will give you this value.
  2. We have a scaling problem. I have 50+ repos in my test environment. This generates a lengthy dropdown menu which scrolls offscreen. And it really won't scale for those with hundreds or thousands of repos so we need a better plan. The bootstap in-use is really old and we don't have scrollable dropdown menus. :( I think we need to list only the repos that are in the infiltered My Tickets resultset. That means we need to execute two Lucene queries. Hopefully the performance impact will be minor compared to the entire page-loading cycle.
@jeyoung

This comment has been minimized.

Contributor

jeyoung commented May 22, 2015

@gitblit, re-submitted with changes based on your code review. Also changed the repository options to constrain to those relevant to the user's watched tickets.

(Sorry about the changes of tabs to spaces, please ignore white-space differences when you compare.)

@Override
public void populateItem(final Item<RepositoryModel> item) {
final RepositoryModel r = item.getModelObject();
PageParameters params = queryParameters(queryParam, milestoneParam, statiiParam, assignedToParam, sortBy, desc, r.getRID(), 1);

This comment has been minimized.

@gitblit

gitblit May 23, 2015

Owner
String rid = r == noneChoice ? null : r.getRID();

Then use rid in the queryParameters call.

final List<QueryResult> tickets =
query(initializeQueryBuilder(defaultQueryParam, username), 1, Integer.MAX_VALUE, sortBy, desc);
final List<RepositoryModel> repositoryChoices = correspondingRepositories(tickets);
Collections.sort(repositoryChoices, new Comparator<RepositoryModel>() {

This comment has been minimized.

@gitblit

gitblit May 23, 2015

Owner
Collections.sort(repositoryChoices);

RepositoryModel is already Comparable.

// by repository
final List<QueryResult> tickets =
query(initializeQueryBuilder(defaultQueryParam, username), 1, Integer.MAX_VALUE, sortBy, desc);

This comment has been minimized.

@gitblit

gitblit May 23, 2015

Owner

query(initializeQueryBuilder(Lucene.watchedby.matches(username), username), 1, Integer.MAX_VALUE, sortBy, desc);

}
final String username = currentUser.getName();
final String defaultQueryParam = "watchedby:"+username;

This comment has been minimized.

@gitblit

gitblit May 23, 2015

Owner

Eliminate.

final String[] statiiParam = (params == null) ? TicketsUI.openStatii : params.getStringArray(Lucene.status.name());
final String assignedToParam = (params == null) ? "" : params.getString(Lucene.responsible.name(), null);
final String milestoneParam = (params == null) ? "" : params.getString(Lucene.milestone.name(), null);
final String queryParam = (params == null || StringUtils.isEmpty(params.getString("q", null))) ? defaultQueryParam : params.getString("q", null);

This comment has been minimized.

@gitblit

gitblit May 23, 2015

Owner
final String queryParam = (params == null) ? "" : params.getString("q");

Gitblit is primarily for those who do, not those who watch; I want the default query to be for developers.

This comment has been minimized.

@jeyoung

jeyoung May 23, 2015

Contributor

In this case, I think mentions:current_user should be part of the default filter.

This comment has been minimized.

@jeyoung

jeyoung May 23, 2015

Contributor

BTW, there seems to be a bug in the mention detection. The mention is added to the list only if @username appears in the middle of the comment. If the comment starts with @username, it is not detected. Looks like it can be fixed with tweaking the regex.

Also, shouldn't the main ticket body also be processed for mentions instead of just comments?

@gitblit

This comment has been minimized.

Owner

gitblit commented May 23, 2015

Good work. We are very close now and this will be a nice usability improvement.

Applied changes according to code review comments.
Removed default "watchedby" filter so that the default now includes "createdby", "responsible", watchedby" and "mention"
@jeyoung

This comment has been minimized.

Contributor

jeyoung commented May 23, 2015

@gitblit These changes should be it. If no more review comments, I'll proceed with fixing the mentions bug, err, mentioned above.

@gitblit gitblit merged commit c33ac10 into gitblit:develop May 27, 2015

@gitblit

This comment has been minimized.

Owner

gitblit commented May 27, 2015

Thanks Eddy! Merged.

@jeyoung

This comment has been minimized.

Contributor

jeyoung commented May 27, 2015

A pleasure. I'd really like to look into implementing tickets across groups. Is that on the roadmap for 1.7.0? Otherwise, I can pick another 1.7.0 item.

@jeyoung jeyoung deleted the jeyoung:ticket-57_develop branch May 27, 2015

@gitblit

This comment has been minimized.

Owner

gitblit commented May 27, 2015

Tickets are intricately tied to repositories. It would take some heroic changes (i.e. likely not mergeable) to accommodate that.

If you really wanted to explore that I would suggest implementing some sort of ticket-container object with it's own persistence mechanism and UI. Maybe this could be a plugin but I haven't spent more than 30 seconds thinking about this so maybe that is a dumb idea. ;)

@jeyoung

This comment has been minimized.

Contributor

jeyoung commented May 27, 2015

Hmm. I forgot about the integration of tickets within repositories, but your idea of a “repository group ticket service” is worth pursuing. I’ll think about that a bit more this week-end.

From: James Moger [mailto:notifications@github.com]
Sent: 27 May 2015 22:18
To: gitblit/gitblit
Cc: Eddy Young
Subject: Re: [gitblit] Implemented filtering by repository on my tickets page (#259)

Tickets are intricately tied to repositories. It would take some heroic changes (i.e. likely not mergeable) to accommodate that.

If you really wanted to explore that I would suggest implementing some sort of ticket-container object with it's own persistence mechanism and UI. Maybe this could be a plugin but I haven't spent more than 30 seconds thinking about this so maybe that is a dumb idea. ;)


Reply to this email directly or view it on GitHub #259 (comment) . https://github.com/notifications/beacon/AAEGeQ7SfcrFMsr3E7Ox_AOTwonZBlYoks5oNiwFgaJpZM4Eg_wa.gif

gitblit added a commit that referenced this pull request Jun 15, 2015

@fzs fzs modified the milestone: 1.7.0 Mar 18, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment