Summary

Initial code for tracking user changes

Review Request #2891 — Created Feb. 18, 2012 and discarded Sept. 22, 2012, 4:52 p.m.

Information

Owner

weilul

Repository

Review Board

Branch

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

Added one field "change_user" to changedesc model.
Modify evolution files related.

Testing Done

Unsucessful. Evolution did not happen to the database.

Screenshots

users (weilul, willer) got trakced

multipe draft, owned by separate users

Issues

Description	From	Last Updated
This must stay. This particular evolution is only for mysql. The sequences code should look like: SEQUENCE = [] if …	chipx86	Feb. 18, 2012, 7:24 a.m.
This stuff that got axed here - I think we might want to put it back. The "fields_changed_longtext" evolution should …	mike_conley	Feb. 18, 2012, 7:24 a.m.
This line is longer than 80 characters.	ME medanat	Feb. 25, 2012, 1:16 a.m.
This looks like its longer than 80 characters.	ME medanat	Feb. 25, 2012, 1:16 a.m.
Both these lines are longer than 80 characters.	ME medanat	Feb. 25, 2012, 1:18 a.m.
This line is longer than 80 characters.	ME medanat	Feb. 25, 2012, 1:18 a.m.
whitespace	mike_conley	April 6, 2012, 9:15 a.m.
I'm not sure if 'change_user' is appropriately evocative of the purpose of this field. Perhaps 'last_modified_by'?	AM ammok	April 14, 2012, 3:20 p.m.
Should this be 'draft_creator' instead of 'user'?	AM ammok	April 14, 2012, 3:20 p.m.
This is a little nit-picky as it doesn't usually affect the rendering, but there are extraneous spaces before "Review" and …	AM ammok	April 14, 2012, 3:20 p.m.
This line is a bit too long.	AM ammok	April 14, 2012, 3:20 p.m.
This line is a bit too long.	AM ammok	April 14, 2012, 3:20 p.m.

reviewboard/changedescs/evolutions/__init__.py (Diff revision 1)

The issue has been resolved. Show all issues

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',
    ]

When I made the below changes, this evolution applied cleanly for me.

reviewboard/changedescs/evolutions/__init__.py (Diff revision 1)

The issue has been resolved. Show all issues

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')

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)

Show changes

	reviewboard/changedescs/models.py
	reviewboard/changedescs/evolutions/__init__.py
	reviewboard/changedescs/evolutions/change_desc_user.py

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)

Show changes

	reviewboard/changedescs/models.py
	reviewboard/changedescs/evolutions/__init__.py
	reviewboard/changedescs/evolutions/change_desc_user.py
	reviewboard/reviews/models.py
	reviewboard/reviews/evolutions/__init__.py
	reviewboard/reviews/evolutions/track_user_changes.py

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)
The issue has been resolved. Show all issues
```
This line is longer than 80 characters.
```
reviewboard/changedescs/models.py (Diff revision 3)
The issue has been resolved. Show all issues
```
This looks like its longer than 80 characters.
```

reviewboard/reviews/evolutions/track_user_changes.py (Diff revision 3)

The issue has been dropped. Show all issues

Both these lines are longer than 80 characters.

weilul Feb. 25, 2012, 1:19 a.m.

I recently rolled back to a previous revision so this issue is no longer present. Thanks anyway.

reviewboard/reviews/models.py (Diff revision 3)
The issue has been dropped. Show all issues
```
This line is longer than 80 characters.
```
1. WE
  
  weilul Feb. 25, 2012, 1:19 a.m.
  The same as above.

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)

Show changes

	reviewboard/changedescs/models.py
	reviewboard/changedescs/evolutions/__init__.py
	reviewboard/changedescs/evolutions/change_desc_user.py
	reviewboard/reviews/models.py
	reviewboard/templates/reviews/review_detail.html
	reviewboard/webapi/resources.py

Change Summary:: screen shot added
Screenshots:: users (weilul, willer) got trakced

Change Summary:

Fixed ">80 chars in a line" issues Yazan proposed.

Diff:

Revision 5 (+16 -3)

Show changes

	reviewboard/changedescs/models.py
	reviewboard/changedescs/evolutions/__init__.py
	reviewboard/changedescs/evolutions/change_desc_user.py
	reviewboard/reviews/models.py
	reviewboard/templates/reviews/review_detail.html
	reviewboard/webapi/resources.py

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.

weilul March 6, 2012, 12:44 p.m.

I will add multiple user support soon.

But we also have race/override problem (R1R2W1W2 -> 2 wins).
Christian/Mike suggests: 1) make an explicit alert to prevent overriding, or, 2) using transaction control; they prefer 1)

How do you think?

chipx86 March 6, 2012, 5:09 p.m.

I still don't think we want that.

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".

Don't block submission, or display an alert, or do anything fancy. Just inform the user and let them decide whether to hold off or whatever.

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

Diff:

Revision 6 (+33 -7)

Show changes

	reviewboard/changedescs/models.py
	reviewboard/changedescs/evolutions/__init__.py
	reviewboard/changedescs/evolutions/change_desc_user.py
	reviewboard/reviews/models.py
	reviewboard/reviews/evolutions/__init__.py
	reviewboard/reviews/evolutions/track_user_changes.py
	reviewboard/templates/reviews/review_detail.html
	reviewboard/webapi/resources.py

Screenshots:

multipe draft, owned by separate users

Fix it!

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

weilul April 12, 2012, 11:20 p.m.

Hey Mike,

1) Ok, will do.
2) Not yet.
3) That sentance is out of date. Evolution applied successfully. I am just struggling with testing.

-Willer

reviewboard/reviews/models.py (Diff revision 6)
An issue was opened. Show all issues
```
whitespace
```

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)

Show changes

	reviewboard/attachments/forms.py
	reviewboard/changedescs/models.py
	reviewboard/changedescs/evolutions/__init__.py
	reviewboard/changedescs/evolutions/change_desc_user.py
	reviewboard/reviews/forms.py
	reviewboard/reviews/models.py
	reviewboard/reviews/evolutions/__init__.py
	reviewboard/reviews/evolutions/track_user_changes.py
	2 more

Fix it!

reviewboard/changedescs/models.py (Diff revision 7)

An issue was opened. Show all issues

I'm not sure if 'change_user' is appropriately evocative of the purpose of this field. Perhaps 'last_modified_by'?

reviewboard/reviews/models.py (Diff revision 7)
An issue was opened. Show all issues
```
Should this be 'draft_creator' instead of 'user'?
```

reviewboard/templates/reviews/review_detail.html (Diff revision 7)

An issue was opened. Show all issues

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.

reviewboard/webapi/resources.py (Diff revision 7)
An issue was opened. Show all issues
```
This line is a bit too long.
```
reviewboard/webapi/resources.py (Diff revision 7)
An issue was opened. Show all issues
```
This line is a bit too long.
```

Status:: Discarded