-
-
reviewboard/changedescs/evolutions/__init__.py (Diff revision 1) This must stay. This particular evolution is only for mysql. The sequences code should look like: SEQUENCE = [] if settings.....mysql: .... fields_changed_longtext SEQUENCE += [ 'change_desc_user', ]
Initial code for tracking user changes
Review Request #2891 — Created Feb. 18, 2012 and discarded
Added one field "change_user" to changedesc model. Modify evolution files related.
Unsucessful. Evolution did not happen to the database.
| Description | From | Last Updated |
|---|---|---|
|
This must stay. This particular evolution is only for mysql. The sequences code should look like: SEQUENCE = [] if … |
 chipx86
|
|
|
This stuff that got axed here - I think we might want to put it back. The "fields_changed_longtext" evolution should … |
 mike_conley
|
|
|
This line is longer than 80 characters. |
ME medanat | |
|
This looks like its longer than 80 characters. |
ME medanat | |
|
Both these lines are longer than 80 characters. |
ME medanat | |
|
This line is longer than 80 characters. |
ME medanat | |
|
whitespace |
mike_conley
|
|
|
I'm not sure if 'change_user' is appropriately evocative of the purpose of this field. Perhaps 'last_modified_by'? |
AM ammok | |
|
Should this be 'draft_creator' instead of 'user'? |
AM ammok | |
|
This is a little nit-picky as it doesn't usually affect the rendering, but there are extraneous spaces before "Review" and … |
AM ammok | |
|
This line is a bit too long. |
AM ammok | |
|
This line is a bit too long. |
AM ammok |
-
When I made the below changes, this evolution applied cleanly for me.
-
reviewboard/changedescs/evolutions/__init__.py (Diff revision 1) This stuff that got axed here - I think we might want to put it back. The "fields_changed_longtext" evolution should *only* run when using a MySQL database. So what we want is: from django.conf import settings SEQUENCE = [] if settings.DATABASES['default']['ENGINE'].endswith('mysql'): SEQUENCE.append('fields_changed_longtext') SEQUENCE.append('change_desc_user')
WE
Change Summary:
Fixed the evolution/database issue that Christian and Mike raised. Found new field "change_user_id" in the database.
Diff: |
Revision 2 (+10) |
|---|
WE
Change Summary:
Added field "draft_creator" to model "review_request_draft". Changed field "review_request" in model "review_request_draft". Evolution is successful.
Diff: |
Revision 3 (+20 -2) |
|---|
ME
-
I'm not familiar with django evolutions, but I can point out some style issues. -Yazan
-
reviewboard/changedescs/evolutions/change_desc_user.py (Diff revision 3) This line is longer than 80 characters.
-
-
reviewboard/reviews/evolutions/track_user_changes.py (Diff revision 3) Both these lines are longer than 80 characters.
-
WE
Change Summary:
I felt it was unnecessary to add the field "draft_creator", so I discarded all changes in diff revision 3. After rolling back to diff revision 2, I successfully made the "track user changes" feature and successfully manually tested it out on my local dev server. Attached is the screen shot where you can find two different users (weilul, willer) were recorded when they made the changes.
Diff: |
Revision 4 (+14 -3) |
|---|
-
This looks like it will record whoever hit "publish", but won't track anything up to that point. In the initial change, I believe Waleed had started to add the ability for there to be multiple concurrent drafts, owned by separate users.
WE
Change Summary:
Status: - Added "multiple concurrent draft, owned by separate users" feature according to David's last comment. See attached screen shot. - unique_together = (review_request, user) - Multiple users have a race problem here; the second overrides the first. - Manually tested on local dev server; no problems in creating, editing and publishing the draft. - Got 76 errors and 3 failures in unit test. Next step: - Analyze the errors and failures - Most errors are related to "get_object_or_none() returning multiple users"; need to look up all ".draft". Future: - Change "draft_creator" to "user"; it is good enough - Probably make an alert feature to prevent the race problem mentioned above
-
Hey Willer, I think we should get an update going on this project. 1) It looks like it's been bitrotted by other changes in master. Can you update the patch? 2) What about ChipX86's request, "What I really just want to see is a banner above the review request when there's a draft owned by someone other than you saying "<User> [, <user>, ...] is making changes to this review request"." <-- has that been done? 3) In your "testing done" section, you write that the evolutions never successfully applied...is this still true? -Mike
-
WE
Change Summary:
Repeatedly ran unit test and fixed errors. Have fixed all errors except for those in test cases. The reason why these test cases have errors: - ReviewRequestDraft.create() has one more argument, "user" - The test cases are out of date; they call ReviewRequestDraft.create() without the "user" argument In order to eliminate these errors, I may have to change test cases. Before I do so, can you guys give me any comments? Thanks, Willer
Diff: |
Revision 7 (+43 -15)
|
|---|
AM
-
-
reviewboard/changedescs/models.py (Diff revision 7) I'm not sure if 'change_user' is appropriately evocative of the purpose of this field. Perhaps 'last_modified_by'?
-
-
reviewboard/templates/reviews/review_detail.html (Diff revision 7) This is a little nit-picky as it doesn't usually affect the rendering, but there are extraneous spaces before "Review" and after the period.
-
-