- Change Summary:
-
Fixed the unit tests errors and failures.
- Diff:
-
Revision 2 (+108 -47)
Multi user collaboration on review requests
Review Request #3242 — Created July 22, 2012 and discarded
This is what I have so far for the multi user collaboration on review requests. So far, what I have done is allow all reviewers (users in the target people and groups) to be allowed to change the review request. This would all the reviewers to all the normal changing review request functionalities. I'm not sure if we have decided who can be able to modify the review request. For now, I put them as all the reviewers. Also, I've included the last_updated_by field to the review request model in order to display who has last modified the review request or published new review on the dashboard.
Manual testing. At first, I've ran the unit tests and had a bunch of errors and one failure. The errors are all expected due to the change of the ReviewRequestDraft.create() function and I need to look into that one failure. I think it's expected failure though. As of diff 2, all the test failures/errors have been fixed. Manual testing include: - Creating new review requests and changing submitter to another account. Permissions to edit the review request behaved correctly for new user - The original author correctly still has permission. - Changing the submitter back to the original submitter. Previous submitter has no permission to edit. - Check Admin. Admin is always able to edit any review request. - Discarding and undiscarding a review request. - Dealing with previous review request that existed before my change. The submitter field is populated correctly. Edit permissions acting properly before changing submitter. Changed submitter and still acting correctly.
| Description | From | Last Updated |
|---|---|---|
|
Two blank lines. |
 chipx86
|
|
|
Should be one line, like: """A column ...""" |
chipx86
|
|
|
Can you use super() here, instead of old-style Column initialization? |
chipx86
|
|
|
Can be shortened to: return obj.last_updated_by or "" |
chipx86
|
|
|
Two blank lines. |
chipx86
|
|
|
Blank line isn't needed. |
chipx86
|
|
|
I think we need to figure out what the rules should be for modifying a review request. I don't think … |
chipx86
|
|
|
This will fail for any drafts that exist at the time of deployment of this change. We should do an … |
chipx86
|
|
|
Same here. |
chipx86
|
|
|
Keep the blank line. |
chipx86
|
|
|
No spaces in the {{}} Should remove the space after the "." (and maybe remove the period, since we didn't … |
chipx86
|
|
|
I'm a bit worried about the pk= for these User queries in the tests. Can you change them instead to … |
chipx86
|
|
|
CODE T?M B?Y R?I CHA N?I |
RO ronaldoit | |
|
This really can just be one evolution. We tend to do one per feature/change. .. And actually, I don't see … |
chipx86
|
|
|
Can you add a trailing comma? One less modified line next time something is added. |
chipx86
|
|
|
Mind changing this to use parens instead of \ ? |
chipx86
|
|
|
No need for \, since it's in parens. |
chipx86
|
|
|
No need for \ |
chipx86
|
|
|
While you're here, mind changing them to === ? (We shouldn't be using ==, but I was young and naïve … |
chipx86
|
|
|
Indentation looks wrong. |
chipx86
|
|
|
Two blank lines. |
chipx86
|
|
|
!== |
chipx86
|
|
|
Keep the blank line. Two between things. |
chipx86
|
|
|
Is it worth folding the permission into that variable? |
chipx86
|
CR
- Change Summary:
-
Updated "Testing Done" details.
- Summary:
-
Multi user collaboration on review requests [DRAFT]Multi user collaboration on review requests
- Testing Done:
-
~ Manual testing. I've ran the unit tests and had a bunch of errors and one failure. The errors are all expected due to the change of the ReviewRequestDraft.create() function and I need to look into that one failure. I think it's expected failure though.
~ Manual testing. At first, I've ran the unit tests and had a bunch of errors and one failure. The errors are all expected due to the change of the ReviewRequestDraft.create() function and I need to look into that one failure. I think it's expected failure though.
~ I'll go through them all and make modifications/fixes as necessary.
~ As of diff 2, all the test failures/errors have been fixed.
-
Hey. This is looking great. As I mention below, we need to figure out what the rules should be for who can modify another user's review requests, because I don't think target reviewers is quite correct. Let's discuss it.
-
-
-
-
-
-
-
I think we need to figure out what the rules should be for modifying a review request. I don't think necessarily that every user listed should have the ability (and this query is expensive anyway). My feeling is that the ability to modify someone else's review request should be pretty limited, and may even be something we want to be opt-in in an organization (or part of an organization). Perhaps we should have a discussion in IRC about what's most correct here.
-
This will fail for any drafts that exist at the time of deployment of this change. We should do an || on these.
-
-
-
No spaces in the {{}} Should remove the space after the "." (and maybe remove the period, since we didn't have it before) -
I'm a bit worried about the pk= for these User queries in the tests. Can you change them instead to look up by username? (This and all others.)
CR
- Change Summary:
-
Modified is_mutable_by method so that it doesn't query for reviewers' groups or people anymore. Basically reverted back to where it was before. When we decide on what type of users can modify the review request, we can modify that method again to accommodate the multi user collaboration. I still really like the idea of having the idea where the submitter can add a list of other users as owners of the ReviewRequest. I think that is simple and intuitive but may take a bit of work to redo the UI on the ReviewRequest. I love to hear suggestions or feedback! :)
- Diff:
-
Revision 3 (+126 -45)
CR
- Change Summary:
-
Allowed to change the owner of a ReviewRequest. The field 'Submitter' into the UI is changed to 'Owner'. Refactored some of the JS to use the jQuery autocompleter for the owner field as well. Added the ability of the owner to be able to modify a ReviewRequest. The submitter is now shown right underneath the summary and beside the created time.
- Diff:
-
Revision 4 (+236 -64)
-
Spotted a couple things, but it's very close, and I'm hopeful we can get this into 1.7 if you feel good about it. Remind me, is it just admins that can create drafts, or can users with the can_edit permission also do it?
-
This really can just be one evolution. We tend to do one per feature/change. .. And actually, I don't see the other file referenced?
-
-
-
-
-
-
While you're here, mind changing them to === ? (We shouldn't be using ==, but I was young and naïve once.)
-
-
-
-
-
CR
- Change Summary:
-
Fixed all the minor issues that were bought up during the last code review. Added a missing evolution file. More manual testing done.
- Diff:
-
Revision 5 (+260 -68)
CR
- Change Summary:
-
Updated testing section.
- Testing Done:
-
Manual testing. At first, I've ran the unit tests and had a bunch of errors and one failure. The errors are all expected due to the change of the ReviewRequestDraft.create() function and I need to look into that one failure. I think it's expected failure though.
As of diff 2, all the test failures/errors have been fixed.
+ + Manual testing include:
+ - Creating new review requests and changing submitter to another account. Permissions to edit the review request behaved correctly for new user + - The original author correctly still has permission. + - Changing the submitter back to the original submitter. Previous submitter has no permission to edit. + - Check Admin. Admin is always able to edit any review request. + - Discarding and undiscarding a review request. + - Dealing with previous review request that existed before my change. The submitter field is populated correctly. Edit permissions acting properly before changing submitter. Changed submitter and still acting correctly.