View Issue Details

Jump to NotesJump to History
IDProjectCategoryView StatusDate SubmittedLast Update
0027292Open CASCADEWebsite:Trackerpublic2016-03-21 14:312016-08-16 17:35
ReporterabvAssigned Tobugmaster  
PrioritynormalSeverityminor 
Status closedResolutionfixed 
Summary0027292: Mantis - add possibility to get notifications on changes in particular part of code at review stage
DescriptionCurrent process of reviewing changes in OCCT code assumes a single reviewer (by default, the maintainer of the corresponding OCCT component), who is responsible for verification of the code changes. However, in many cases the changes made for one component of OCCT include also modifications in other components (typical case is documentation), which may deserve their own review by expert. It is difficult to define formal rules when this additional review is necessary, and it would be overkill to introduce multiple reviewers.

Thus it is proposed to add a feature allowing the persons who take care of some part of OCCT code (e.g. particular class or package, or module) to get notification when some change in relevant code is submitted for review. This gives good change for such persons ("observers") to check the change and give timely advice if something is to be corrected.

The implementation can be as follows:

- in user's preferences, add a "watch for changes in" field where the user can list a set of patterns to search for (glob-style)

- when some issue is switched to Resolved status, the system should check changes contained in relevant branch (latest starting with CR<issus_number>), and notify people who set watch pattern matching some of the changed paths, by mail

The notification should explicitly indicate that it is about changes in the monitored files submitted for review, and list the files in the commit that matched the pattern.

For instance:

/The branch CRxxxxx is submitted for review. That branch contains changes in files that you set to be monitored in your Preferences in Mantis: ... /

In order to make association of the issue and a branch more explicit, we can add a field "Branch" to be filled by the person when switching issue to Resolved state. That field can be found automatically as the most recent branch with name CRxxxx, with xxxx the ID of the issue. If the branch is not found nor indicated manually, the developer must input something like "no branch" to indicate that no changes are submitted through Git. Naturally, in that case there will be no notifications to observers.
Steps To ReproduceIn Mantis: Go to My Account > Preferences
TagsNo tags attached.
Test case number

Relationships

related to 0024600 closedbugmaster Bugs in the category - automatic notifications for the list of persons 

Activities

aiv

2016-04-25 18:34

reporter   ~0053566

Resolved. Go to My Account > Preferences and find the appropriate configuration option. Your comments/suggestions are welcome.
Option is available for users with global status 'developer' and higher.

aiv

2016-05-23 20:42

reporter   ~0054316

A remark by msv of May 19, 2016:

I have a remark. I received unwanted notification when some branch was pushed for review.
This is the branch CR27317. This branch does not contain any changes in paths that I am
submitted on. However this branch is based on another branch, which I already reviewed
earlier - CR26329. So, my suggestion is to replace the logic that finds diffs between common
ref of master and the inspected branch, and the inspected branch. It should find only
differences of the branch itself, not including the changes of other branches parent for the
inspected one.

aiv

2016-05-23 20:49

reporter   ~0054317

Andrey, Mikhail,

With the approach proposed by Mikhail, I see at least some edge cases. Could you please suggest how to do with them. I created a test repo for illustration.

1. CR23456 has just been pushed.

*   a8b345c (origin/CR23456) Merge branch 'origin/CR12345'
|\
| * d0b07c4 (origin/CR12345) test
| * ccc632c Test commit in new branch
* | dc3b08e test master
|/
* 61f3811 Commit 2
| * e6abcea (origin/master) test master
|/
* 7ab9e04 Test commit
* 6cb5e21 Initial commit

Which commits should be ‘diff’-ed?
I suppose a8b345c and 61f3811.
Am I right?


2. The same, but branch origin/CR12345 is missed.

*   a8b345c (origin/CR23456) Merge branch 'origin/CR12345'
|\
| * d0b07c4 test
| * ccc632c Test commit in new branch
* | dc3b08e test master
|/
* 61f3811 Commit 2
| * e6abcea (origin/master) test master
|/
* 7ab9e04 Test commit
* 6cb5e21 Initial commit

Diff a8b345c and 7ab9e04?


3. CR23456 has just been pushed.

* d3c609f (origin/CR23456) New revision
*   a8b345c Merge branch 'origin/CR12345'
|\
| * d0b07c4 (origin/CR12345) test
| * ccc632c Test commit in new branch
* | dc3b08e test master
|/
* 61f3811 Commit 2
| * e6abcea (origin/master) test master
|/
* 7ab9e04 Test commit
* 6cb5e21 Initial commit

Diff d3c609f and a8b345c?


4. The same, but branch origin/CR12345 is missed.

* d3c609f (origin/CR23456) New revision
*   a8b345c Merge branch 'origin/CR12345'
|\
| * d0b07c4 test
| * ccc632c Test commit in new branch
* | dc3b08e test master
|/
* 61f3811 Commit 2
| * e6abcea (origin/master) test master
|/
* 7ab9e04 Test commit
* 6cb5e21 Initial commit

Diff a8b345c and 7ab9e04?

abv

2016-05-23 21:54

manager   ~0054318

Alexander, we have discussed this today with Mikhail and decided that we shall try living with existing implementation which may perhaps be sometimes too verbose, but at least will not skip relevant events. If the case reported by Mikhail will be recurrent and annoying, he will propose algorithm how to improve, taking into account realistic situations similar to what you have sketched.

msv

2016-05-24 10:36

developer   ~0054323

OK. I agree with Andrey.

Issue History

Date Modified Username Field Change
2016-03-21 14:31 abv New Issue
2016-03-21 14:31 abv Assigned To => aiv
2016-03-21 14:32 abv Relationship added related to 0024600
2016-04-25 18:34 aiv Note Added: 0053566
2016-04-25 18:34 aiv Assigned To aiv => bugmaster
2016-04-25 18:34 aiv Status new => resolved
2016-04-25 18:34 aiv Steps to Reproduce Updated
2016-04-27 11:01 bugmaster Status resolved => closed
2016-04-27 11:01 bugmaster Resolution open => fixed
2016-05-23 20:42 aiv Note Added: 0054316
2016-05-23 20:42 aiv Assigned To bugmaster => aiv
2016-05-23 20:42 aiv Status closed => assigned
2016-05-23 20:49 aiv Note Added: 0054317
2016-05-23 21:54 abv Note Added: 0054318
2016-05-24 10:36 msv Note Added: 0054323
2016-08-16 17:34 aiv Status assigned => closed
2016-08-16 17:35 aiv Assigned To aiv => bugmaster