Feature: Starred groups with option to restrict view to these projects only #74

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
3 participants
@heldinz

heldinz commented Feb 18, 2013

Added a feature for when the application has the "grouped" setting for repositories: users can select particular projects/groups that they are interested in (similar to starring a repository on GitHub) and choose to view only those repositories on the Repositories and Activity pages. Filter parameters are preserved when making changes. The user view preference is saved as a property on the user object and is therefore "sticky".

In code we have gone with the more neutral terms "selected" and "unselected", but in the UI have opted for "starred/unstarred". The current English labels for the view preference toggle are therefore show all repositories and show starred projects only while those for the buttons to select/unselect a project are: ☆ star (unicode white star) and ★ unstar (unicode black star). These can of course be customized according to preference.

A Selenium test is provided.

repositories-page-starred-groups-feature

@gitblit

This comment has been minimized.

Show comment
Hide comment
@gitblit

gitblit Feb 19, 2013

Owner

This looks like a very nice addition. The only thing I am not crazy about is persisting the ui config in the user service. I think that show starred/show all would better be handled by a new Gitblit settings cookie. What do you think of that?

Owner

gitblit commented Feb 19, 2013

This looks like a very nice addition. The only thing I am not crazy about is persisting the ui config in the user service. I think that show starred/show all would better be handled by a new Gitblit settings cookie. What do you think of that?

@srse

This comment has been minimized.

Show comment
Hide comment
@srse

srse Feb 20, 2013

Contributor

As you may use different computers at the same time (like I do - most times I use two in parallel, sometimes three) I think it would be better to have the settings persisted on server side. Otherwise I have to select my favorites three times. Furthermore, developers tend to clear their caches from time to time and if they develop a web app, some of them might need to clear the cached cookies as well and they probably will feel bothered selecting which cookies to delete and which cookies to keep each time. From the perspective of usability for the gitblit user I think it's better to persist the settings on server side.

Other opinions?

Contributor

srse commented Feb 20, 2013

As you may use different computers at the same time (like I do - most times I use two in parallel, sometimes three) I think it would be better to have the settings persisted on server side. Otherwise I have to select my favorites three times. Furthermore, developers tend to clear their caches from time to time and if they develop a web app, some of them might need to clear the cached cookies as well and they probably will feel bothered selecting which cookies to delete and which cookies to keep each time. From the perspective of usability for the gitblit user I think it's better to persist the settings on server side.

Other opinions?

@gitblit

This comment has been minimized.

Show comment
Hide comment
@gitblit

gitblit Feb 25, 2013

Owner

I've reviewed this more thoroughly and here are my thoughts.

  1. I like the idea of starring. I think the language should be "star" all the way down through the code to the user service persistence. "Selected" is ambiguous and can refer to anything. I think we can get by with a tooltip instead of showing star/unstar text along with the icon.
  2. I would like to be able to star individual repositories, not just projects so I would amend the functionality to use regex such that starring a project sets a value like "myprojects/*".
  3. I want to be able to star/unstar a repository from the project view (https://demo-gitblit.rhcloud.com/project/main) and from the repository page in the fork controls (https://demo-gitblit.rhcloud.com/summary/gitblit.git). From the repositories page I should be able to star/unstar projects - just like you have it.
  4. Renaming a repository must also update starred repository settings for all users.
  5. I do not want to rewrite the entire user service file when a user changes their UI view for showing starred/unstarred repositories & projects so I do not support persisting this UI preference in the user service. I would prefer either a cookie or perhaps a separate server-side user preference storage.
  6. When we test for if the user is logged in to show/hide elements currently a null test is sufficient, but this needs to be extended to a test for anonymous too since at some point I want to always have a user object instead of relying on null.
  7. When using the translations within the web UI we must use Wicket's built-in translation service. Wicket's service accounts for the user's browser preference. The Translation class only returns the translation for the server-side locale preference. I know this is confusing because the Translation class is so tempting to use, but that is meant for application code like the Manager. In the future I hope to have this project broken up into a few smaller projects where this is more clear but that is a separate task.
Owner

gitblit commented Feb 25, 2013

I've reviewed this more thoroughly and here are my thoughts.

  1. I like the idea of starring. I think the language should be "star" all the way down through the code to the user service persistence. "Selected" is ambiguous and can refer to anything. I think we can get by with a tooltip instead of showing star/unstar text along with the icon.
  2. I would like to be able to star individual repositories, not just projects so I would amend the functionality to use regex such that starring a project sets a value like "myprojects/*".
  3. I want to be able to star/unstar a repository from the project view (https://demo-gitblit.rhcloud.com/project/main) and from the repository page in the fork controls (https://demo-gitblit.rhcloud.com/summary/gitblit.git). From the repositories page I should be able to star/unstar projects - just like you have it.
  4. Renaming a repository must also update starred repository settings for all users.
  5. I do not want to rewrite the entire user service file when a user changes their UI view for showing starred/unstarred repositories & projects so I do not support persisting this UI preference in the user service. I would prefer either a cookie or perhaps a separate server-side user preference storage.
  6. When we test for if the user is logged in to show/hide elements currently a null test is sufficient, but this needs to be extended to a test for anonymous too since at some point I want to always have a user object instead of relying on null.
  7. When using the translations within the web UI we must use Wicket's built-in translation service. Wicket's service accounts for the user's browser preference. The Translation class only returns the translation for the server-side locale preference. I know this is confusing because the Translation class is so tempting to use, but that is meant for application code like the Manager. In the future I hope to have this project broken up into a few smaller projects where this is more clear but that is a separate task.
@srse

This comment has been minimized.

Show comment
Hide comment
@srse

srse Feb 25, 2013

Contributor

Hi James,

Alice and me just discussed your feedback. we would like to divide the changes to make into two packages. So, we concentrate on the core functionality first, then we merge back to the master, before we extend the UI again.

There is one question remaining regarding 7.: What should we use instead of the Translation class then? Can you give us an example or point us to a line in the existing code, which does it the right way?

We think that we can finish 1., 4., 5., and 6. (and probably 7. if we know how to do it correctly) until end of next week.
I think we should merge then first before we add repository specific selection (regex support) and more UI components to use the feature (2. and 3. of your feedback).
What do you think about this strategy?

Contributor

srse commented Feb 25, 2013

Hi James,

Alice and me just discussed your feedback. we would like to divide the changes to make into two packages. So, we concentrate on the core functionality first, then we merge back to the master, before we extend the UI again.

There is one question remaining regarding 7.: What should we use instead of the Translation class then? Can you give us an example or point us to a line in the existing code, which does it the right way?

We think that we can finish 1., 4., 5., and 6. (and probably 7. if we know how to do it correctly) until end of next week.
I think we should merge then first before we add repository specific selection (regex support) and more UI components to use the feature (2. and 3. of your feedback).
What do you think about this strategy?

@gitblit

This comment has been minimized.

Show comment
Hide comment
@gitblit

gitblit Feb 25, 2013

Owner

That sounds fine.

I'm having a hardtime finding an example in my code. :( You can look at NavigationPanel to see the use of getString(key). But you should also notice that this method is used inside a repeater. Wicket complains obnoxiously if you call getString from the constructor of a panel. In your implementation you would have to do that or pass a Localizer instance from the owning page as a constructor argument.

One alternative is to specify the translation key in your markup using the wicket:message notation but to do that you would have to create two buttons and show/hide buttons. Another alternative would be a dropdown list:
http://twitter.github.com/bootstrap/components.html#buttonDropdowns

I would favor a dropdown here because there may be reasons to add more selections in the future; a toggle button does not scale. Gitblit currently has Bootstrap 2.0.4 (I think) so the docs may be out of sync.

One thing I try to keep tight control of is Wicket page sessions. When you use Wicket links or AJAX you create a serialized version of the page in a user page-session-store. For some pages this is absolute must (Edit Repository) for most, though, I try hard to avoid that and to keep pages session-less.

This is not the same as the servlet container session. A page with Wicket state will lead to link failures if Wicket thinks your page has expired. One way I work around that is by minimizing use of Wicket-generated links by passing explicit urls with parameters REST-ish or by storing important state in the user session. It may not be a problem here, but it is something to keep in mind.

A good example of this is the repositories page for an Admin or Owner and the delete repo links. Those will fail after a session timeout because they use Wicket urls, Javascript, and rely on the a page session store. In contrast, the edit links should always work because they are manually generated and point to REST-ish urls. I'm not in love with Wicket, but it is what we've got.

As for the regex, I don't think you need anything different UI-wise to support regex. It should be straight-forward and internal.
to star a project = user.addStarredThingy("myproject/*")
user.isStarredThingy(string) should do the regex matching like user.hasRepositoryPermission

I also now notice this line:
String rootPath = StringUtils.getRootPath(model.name)

You shouldn't need that. Check out RepositoryModel.projectPath.

Owner

gitblit commented Feb 25, 2013

That sounds fine.

I'm having a hardtime finding an example in my code. :( You can look at NavigationPanel to see the use of getString(key). But you should also notice that this method is used inside a repeater. Wicket complains obnoxiously if you call getString from the constructor of a panel. In your implementation you would have to do that or pass a Localizer instance from the owning page as a constructor argument.

One alternative is to specify the translation key in your markup using the wicket:message notation but to do that you would have to create two buttons and show/hide buttons. Another alternative would be a dropdown list:
http://twitter.github.com/bootstrap/components.html#buttonDropdowns

I would favor a dropdown here because there may be reasons to add more selections in the future; a toggle button does not scale. Gitblit currently has Bootstrap 2.0.4 (I think) so the docs may be out of sync.

One thing I try to keep tight control of is Wicket page sessions. When you use Wicket links or AJAX you create a serialized version of the page in a user page-session-store. For some pages this is absolute must (Edit Repository) for most, though, I try hard to avoid that and to keep pages session-less.

This is not the same as the servlet container session. A page with Wicket state will lead to link failures if Wicket thinks your page has expired. One way I work around that is by minimizing use of Wicket-generated links by passing explicit urls with parameters REST-ish or by storing important state in the user session. It may not be a problem here, but it is something to keep in mind.

A good example of this is the repositories page for an Admin or Owner and the delete repo links. Those will fail after a session timeout because they use Wicket urls, Javascript, and rely on the a page session store. In contrast, the edit links should always work because they are manually generated and point to REST-ish urls. I'm not in love with Wicket, but it is what we've got.

As for the regex, I don't think you need anything different UI-wise to support regex. It should be straight-forward and internal.
to star a project = user.addStarredThingy("myproject/*")
user.isStarredThingy(string) should do the regex matching like user.hasRepositoryPermission

I also now notice this line:
String rootPath = StringUtils.getRootPath(model.name)

You shouldn't need that. Check out RepositoryModel.projectPath.

@gitblit gitblit referenced this pull request May 8, 2013

Closed

System config frontend #87

@gitblit

This comment has been minimized.

Show comment
Hide comment
@gitblit

gitblit May 31, 2013

Owner

Hi Alice & Sarah,

I have implemented a slightly different starring feature - which at the moment does not touch the repositories page, but likely should. Thanks for contributing your ideas and participating in a discussion with me, even though I ultimately decided to implement this slightly differently. Good luck with your scm-manager transition!

-J

Owner

gitblit commented May 31, 2013

Hi Alice & Sarah,

I have implemented a slightly different starring feature - which at the moment does not touch the repositories page, but likely should. Thanks for contributing your ideas and participating in a discussion with me, even though I ultimately decided to implement this slightly differently. Good luck with your scm-manager transition!

-J

@gitblit gitblit closed this May 31, 2013

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