492674 - Create a Bugzilla::ChangeSet object to represent a set of changes in bugs_activity

Status: NEW

(Bugzilla :: Creating/Changing Bugs, enhancement, P1)

Description

[Max Kanat-Alexander]

We should have an object called Bugzilla::ChangeSet that represents not just a single bug change, but a set of changes made at a particular time to a bug. Or we might just want an array of Bugzilla::Change objects, I'm not quite sure. Whatever we decide, a single "Change" should always "unsplit" its contents appropriately (so changes that are longer than 255 characters just end up as one change).

Comment 1

[Max Kanat-Alexander]

I'd also like to see ChangeSet have a ->comment object, so that you can get the comment associated with the ChangeSet.

Comment 2

[Frédéric Buclin]

As this is blocking the "XML bugmail" bug, this should be fixed soon, ideally for 3.8.

Comment 3

[Max Kanat-Alexander]

reed: Are you going to do this for 4.2? If so, you're welcome to take the bug and set the TM back.

Comment 4

[LegNeato]

I have a working WIP for this. Need to clean it up.

Comment 5

[LegNeato]

One thing that is not clear is how I would detect changes that are split. I guess a change that is 255 characters should be merged with the next record if the change time is the same. Of course, there is the chance that it is an individual history record that just happens to be 255...probably not worth worrying about though.

Comment 6

[Max Kanat-Alexander]

(In reply to comment #5)
> One thing that is not clear is how I would detect changes that are split. I
> guess a change that is 255 characters should be merged with the next record if
> the change time is the same. Of course, there is the chance that it is an
> individual history record that just happens to be 255...probably not worth
> worrying about though.

  I'm pretty sure there's already code that does this in Bugzilla, so just use the same existing code.

Comment 7

[LegNeato]

Attachment: WIP patch

I want some feedback to the approach I am taking

Comment 8

[LegNeato]

Note I am asking for feedback, not a review as I haven't finished the patch. For some reason I can't set feedback:?...

Comment 9

[Max Kanat-Alexander]

Okay. Since you asked for just a general response, I won't talk about any code specifics, I'll just give general design feedback.

I think Bugzilla::Change looks good, architecturally.

I don't think we need those mutators on Bugzilla::ChangeSet, and at the worst, if we do need them, they don't need to be public.

get_all traditionally returns every single object of that type, so we don't want a method named that for ChangeSet.

The "change_id" field in the database should just be called "id" on the bugs_activity table.

I'd rather not rename bug_when for now, because other tables are also using that name.

I'm happy to rename fieldid to field_id, but we should do it in a separate bug, probably.

Bugzilla::Activity seems like an unnecessary wrapper around ChangeSet, and seems like it should instead just be logic inside of Bugzilla::Bug.

I don't see anything actually using before/after yet in Bug->activity, so they shouldn't be there in this patch unless they do ultimately get used here.

As far as I can tell about ChangeSet, it should probably be much simpler. In fact, at this point we probably don't even need it, and activity should just return an array of Bugzilla::Change objects. Eventually, we can modify the code to add in ChangeSet objects, but perhaps for now, it would be best to work on a simple patch, in a new blocker of this bug, that just implements Bugzilla::Change in whatever is the simplest way possible.

Comment 10

[LegNeato]

(In reply to comment #9)
> Okay. Since you asked for just a general response, I won't talk about any code
> specifics, I'll just give general design feedback.

Thanks, appreciate it.

> 
> I think Bugzilla::Change looks good, architecturally.

Sweet, I cribbed it off some other object.

> 
> I don't think we need those mutators on Bugzilla::ChangeSet, and at the worst,
> if we do need them, they don't need to be public.

Gone.

> 
> get_all traditionally returns every single object of that type, so we don't
> want a method named that for ChangeSet.

Yeah, that was left over from playing around and is now gone.

> 
> The "change_id" field in the database should just be called "id" on the
> bugs_activity table.

Fixed.

> 
> I'd rather not rename bug_when for now, because other tables are also using
> that name.

Ok, it seemed weird to call it bug_when when the date refers to the change...but I guess that refers to the bug change when as well.

> 
> I'm happy to rename fieldid to field_id, but we should do it in a separate bug,
> probably.

Ok, I just did it to be consistent with the other tables. I can spin it off.

> 
> Bugzilla::Activity seems like an unnecessary wrapper around ChangeSet, and
> seems like it should instead just be logic inside of Bugzilla::Bug.

Yeah, this is the way I did it but I kept running into places in the code where I had to do the same looping and picking through bugs.

> 
> I don't see anything actually using before/after yet in Bug->activity, so they
> shouldn't be there in this patch unless they do ultimately get used here.

Yeah, that part was unfinished and is now working.

> 
> As far as I can tell about ChangeSet, it should probably be much simpler. In
> fact, at this point we probably don't even need it, and activity should just
> return an array of Bugzilla::Change objects. Eventually, we can modify the code
> to add in ChangeSet objects, but perhaps for now, it would be best to work on a
> simple patch, in a new blocker of this bug, that just implements
> Bugzilla::Change in whatever is the simplest way possible.

The code already had a concept of an "operation" as a group of changes in multiple places, so to make other changes minimal the changeset maps to that.

Comment 11

[LegNeato]

Attachment: WIP patch, v2

I did some more work and addressed some (but not all yet) of the comments above

Comment 12

[Max Kanat-Alexander]

(In reply to comment #10)
> Ok, it seemed weird to call it bug_when when the date refers to the
> change...but I guess that refers to the bug change when as well.

  Oh, I totally agree that it's a strange name, but for now it's a consistent name. It's something we'd fix in another bug if we do fix it.

> Ok, I just did it to be consistent with the other tables. I can spin it off.

  Yeah, I'm all for consistency, and I'd love to see more work done on making the database tables consistent. But it should definitely happen separately.

> Yeah, this is the way I did it but I kept running into places in the code where
> I had to do the same looping and picking through bugs.

  Okay. Well, let's start off with just one use case, if possible, and then build up from there. If you keep running hitting this, grab me on IRC and we'll talk about it.

> The code already had a concept of an "operation" as a group of changes in
> multiple places, so to make other changes minimal the changeset maps to that.

  Yeah, I agree that that is valid and useful, but I think we should first make a patch that adds just Change only, and then after that, add a patch that does ChangeSet.

Comment 13

[LegNeato]

I picked this up again. Sorry for the lag :-/

Comment 14

[Frédéric Buclin]

We are going to branch for Bugzilla 4.4 next week and this bug is either too invasive to be accepted for 4.4 at this point or shows no recent activity. The target milestone is reset and will be set again *only* when a patch is attached and approved.

I ask the assignee to reassign the bug to the default assignee if you don't plan to work on this bug in the near future, to make it clearer which bugs should be fixed by someone else.