Community
Participate
Working Groups
Instead of using an italic font show a little lock icon to the right of the comment header to denote private comments. The lock icon should also support making a comment private in the future.
I changed the Milestone because I include an action to make a comment private. Steffen, if you think we should extract the UI part without the action I can create an patch for this.
Created attachment 198232 [details] patch V1 Here is what I have committed to HEAD. I include an feature that as soon we find the first private comment the UI get activated. You find an checkbox for activating under Additional Settings in the Bugzilla Repository Settings.
Created attachment 198233 [details] mylyn/context/zip
The committed change causes some problems: The comment part is now below the New Comment section and the header font is no longer bold. Can you fix that? I don't see why the new API, TaskAttribute.COMMENT_ID and ITaskComment.setCommentID() are needed. TaskCommentMapper already had a notion of a comment based on the value of the task attribute: comment.setCommentId(mapper.getValue(taskAttribute)). There must only be one way of specifying the ID otherwise the API will be very difficult to use.
(In reply to comment #4) > The committed change causes some problems: The comment part is now below the New > Comment section and the header font is no longer bold. Can you fix that? Font problem is now fixed. I can not reproduce the order problem. > I don't see why the new API, TaskAttribute.COMMENT_ID and > ITaskComment.setCommentID() are needed. TaskCommentMapper already had a notion > of a comment based on the value of the task attribute: > comment.setCommentId(mapper.getValue(taskAttribute)). There must only be one way > of specifying the ID otherwise the API will be very difficult to use. Bugzilla use an unique ID to store this in the database. This ID is used in the UI for manipulation functions. The already used ID is only the unique ID within an bug and starts in every bug with one. Should rename the older ID to number? Thoughts?
Thanks Frank. Both problems seem to be fixed to me. (In reply to comment #5) > Bugzilla use an unique ID to store this in the database. This ID is used in the > UI for manipulation functions. The already used ID is only the unique ID within > an bug and starts in every bug with one. Couldn't we set the attribute value to the ID that is used by Bugzilla? > Should rename the older ID to number? Isn't Bugzilla already setting the value returned by ITaskComment.getNumber()? > > Thoughts?
(In reply to comment #6) > Thanks Frank. Both problems seem to be fixed to me. > > (In reply to comment #5) > > Bugzilla use an unique ID to store this in the database. This ID is used in the > > UI for manipulation functions. The already used ID is only the unique ID within > > an bug and starts in every bug with one. > > Couldn't we set the attribute value to the ID that is used by Bugzilla? No because we need the local number for the replay action bot the ID for the toggle private action > > > Should rename the older ID to number? > > Isn't Bugzilla already setting the value returned by ITaskComment.getNumber()? Yes but this is only for the local number. > > > > Thoughts?
(In reply to comment #7) > > Couldn't we set the attribute value to the ID that is used by Bugzilla? > > No because we need the local number for the replay action both the ID for the > toggle private action It looks BugzillaConnectorUi.getReplyText() is using the number and not the ID. > > > Should rename the older ID to number? > > > > Isn't Bugzilla already setting the value returned by ITaskComment.getNumber()? > Yes but this is only for the local number. This is the part of the code that makes me wonder why we store the commentNum twice: TaskAttribute attribute = repositoryTaskData.getRoot().createAttribute( TaskAttribute.PREFIX_COMMENT + commentNum); TaskCommentMapper taskComment = TaskCommentMapper.createFrom(attribute); taskComment.setCommentId(comment.id + ""); //$NON-NLS-1$ taskComment.setNumber(commentNum); Why not use the ID instead and remove the commentId attribute? TaskAttribute attribute = repositoryTaskData.getRoot().createAttribute( TaskAttribute.PREFIX_COMMENT + comment.id); TaskCommentMapper taskComment = TaskCommentMapper.createFrom(attribute); taskComment.setNumber(commentNum);
(In reply to comment #8) > (In reply to comment #7) > > > > Should rename the older ID to number? > > > > > > Isn't Bugzilla already setting the value returned by ITaskComment.getNumber()? > > Yes but this is only for the local number. > > This is the part of the code that makes me wonder why we store the commentNum > twice: > > TaskAttribute attribute = repositoryTaskData.getRoot().createAttribute( > TaskAttribute.PREFIX_COMMENT + commentNum); > TaskCommentMapper taskComment = > TaskCommentMapper.createFrom(attribute); > taskComment.setCommentId(comment.id + ""); //$NON-NLS-1$ > taskComment.setNumber(commentNum); > > Why not use the ID instead and remove the commentId attribute? > > TaskAttribute attribute = repositoryTaskData.getRoot().createAttribute( > TaskAttribute.PREFIX_COMMENT + comment.id); > TaskCommentMapper taskComment = > TaskCommentMapper.createFrom(attribute); > taskComment.setNumber(commentNum); Steffen, comment.id is the unique id (tag <commentid>) within the Bugzilla installation for an comment. Here is what we get in the xml. <long_desc isprivate="0"> <commentid>2009</commentid> <who name="Mylyn Test">tests</who> <bug_when>2011-09-11 20:57:29 +0200</bug_when> <thetext>Description</thetext> </long_desc> but commentNum is the local number within an bug and starts in every bug with one. We need the commentNum for the "(In reply to comment #commentNum)" and the comment.id is used for set/reset the private flag of an comment.
Frank, please take a look at http://review.mylyn.org/#change,278. I reverted the API changes. In my opinion these are not required but it's sufficient to store the comment ID in the task attribute value. For testing I enabled private comments for the bugs40 instance (the admin account is in the insider group). I noticed that changing the visibility of comments worked but it failed for descriptions. I tried with this bug: http://mylyn.org/bugs40/show_bug.cgi?id=29. Do we have any unit tests for this functionality? I have moved the subtasks under bug 371158. I would like to move the support for private comments from the Bugzilla connector to the framework but I am not sure that we'll be able to do this for 3.7.
Steffen, I have an new review http://review.mylyn.org/279 because the http://review.mylyn.org/278 is no longer open. What I implement is the support for the Description and reset of the use private comments property when the username or the repository URL is changed. All test are passed wit my local environment.
Created attachment 210886 [details] mylyn/context/zip
I had already merged the changes since I had another commit that depended on them. Can you rebase your latest changes against master and push another patch set?
I have diffed the commit against the current master and pushed a rebased patch set. Can you take a look if that has all expected changes? We should fix the support for the description but I'm not sure about the other changes. We shouldn't silently change a setting when the username is modified. Can you take a pass through the patch and remove stuff that's not related to fixing support for private descriptions or bug 349771? Everything else should be moved to a new review bug 371158.
(In reply to comment #14) > I have diffed the commit against the current master and pushed a rebased patch > set. Can you take a look if that has all expected changes? Yes this is what I want. > > We should fix the support for the description but I'm not sure about the other > changes. We shouldn't silently change a setting when the username is modified. The insidergroup is user specific so I thought that we should set the usage of the private comments to false. > Can you take a pass through the patch and remove stuff that's not related to > fixing support for private descriptions or bug 349771? Everything else should > be moved to a new review bug 371158. The changes in BugzillaRepositorySettingsPage.java are the only stuff that is not related to this bug. The main change is that the description is now an TaskAttribute with sub attributes like the comments.
(In reply to comment #15) > > We should fix the support for the description but I'm not sure about the other > > changes. We shouldn't silently change a setting when the username is modified. > > The insidergroup is user specific so I thought that we should set the usage of > the private comments to false. I see your point. I still think it's too weird to flip the setting as a side effect of entering text. Ideally we wouldn't have to bother the user with that setting at all. Please revert the change for now since it's an advanced feature and users who enable it should be aware that they need to have the privileges in order to post private comments. > > Can you take a pass through the patch and remove stuff that's not related to > > fixing support for private descriptions or bug 349771? Everything else should > > be moved to a new review bug 371158. > The changes in BugzillaRepositorySettingsPage.java are the only stuff that is > not related to this bug. Ok. It looked like some of the changes were previously proposed for bug 349771. It doesn't really matter to me how we track this but we should just close bug 349771 if we are done with that. > The main change is that the description is now an TaskAttribute with sub > attributes like the comments. We have to find a different way of doing this, e.g. using a meta-data or only using a sub-attribute for the description in Bugzilla. Please don't commit the task framework changes. The chances for regressions are far to high and we have to iterate over the API first. Generally, all framework and API changes need to be proposed on a separate bug and review that is marked as [api].
Steffen, I create review http://review.mylyn.org/281
Created attachment 210894 [details] mylyn/context/zip
Looks good to me! Do we have any test cases for submitting private comments and descriptions?
(In reply to comment #19) > Looks good to me! Do we have any test cases for submitting private comments and > descriptions? OK, I change some Strings to be IBugzilla Constants. Should I change all repositories so that the admin user is in the insider group or should we define an new user. We need this if we want to have some junit tests.
(In reply to comment #20) > OK, I change some Strings to be IBugzilla Constants. > Should I change all repositories so that the admin user is in the insider group > or should we define an new user. We need this if we want to have some junit > tests. We shouldn't use the admin user since we don't make those credentials public. We should use the tests user or create another user. Do we have a guest@mylyn.eclipse.org account on the repositories?
(In reply to comment #21) . . . > We shouldn't use the admin user since we don't make those credentials public. > We should use the tests user or create another user. Do we have a > guest@mylyn.eclipse.org account on the repositories? I think we not should give the guest account the insider permission. Should I create an insider@mylyn.eclipse.org account?
(In reply to comment #22) > > We shouldn't use the admin user since we don't make those credentials public. > > We should use the tests user or create another user. Do we have a > > guest@mylyn.eclipse.org account on the repositories? > > I think we not should give the guest account the insider permission. > Should I create an insider@mylyn.eclipse.org account? Sure, we could consider that. We already have support for a guest account in the credentials API that's why I suggested that name. Any reason not to use the tests@mylyn.eclipse.org account?
(In reply to comment #23) > (In reply to comment #22) > > > We shouldn't use the admin user since we don't make those credentials public. > > > We should use the tests user or create another user. Do we have a > > > guest@mylyn.eclipse.org account on the repositories? > > > > I think we not should give the guest account the insider permission. > > Should I create an insider@mylyn.eclipse.org account? > > Sure, we could consider that. We already have support for a guest account in > the credentials API that's why I suggested that name. Any reason not to use the > tests@mylyn.eclipse.org account? OK I think we should first try to use the tests@mylyn.eclipse.org before we create an new one.
Steffen, I create some junit tests and pushed an new Patchset to http://review.mylyn.org/#change,281 with two comments.
Thanks. I have added a few comments. One think that I didn't quite understand is the difference between isprivate and defined_isprivate?
(In reply to comment #26) > Thanks. I have added a few comments. One think that I didn't quite understand is > the difference between isprivate and defined_isprivate? Bugzilla use defined_isprivate_ to indicate that you can change the private state and isprivate_ hold the value.
Create Patchset 3 for fix of the comments
Created attachment 211230 [details] screenshot
Looks good! Please feel free to commit. We'll need to update test repositories accordingly.
(In reply to comment #30) > Looks good! Please feel free to commit. We'll need to update test repositories > accordingly. Review now in HEAD! The repositories are changed. test@mylyn.eclipse.org is now member of the insider group.
Created attachment 211237 [details] mylyn/context/zip
