View Issue Details

This bug affects 1 person(s).
 6
IDProjectCategoryView StatusLast Update
18148Feature requests_ Unknownpublic2022-06-02 15:28
ReporterDenisChenu Assigned Toc_schmitz  
PrioritynoneSeverityfeature 
Status resolvedResolutionfixed 
Summary18148: Report > Pull request > assigned > review etc process
Description

A staring point for process discusssion

Additional Information

The start questions are :

The assign must be updated or not,

Gabriel opinion : set it to testing or review but keep assigned to original dev
My opinion : set it to testing or review but remove current assigned (history is there)

Else when ready : review or testing ? Review is just after assigned then …

TagsNo tags attached.
Bug heat6

Users monitoring this issue

User List There are no users monitoring this issue.

Activities

DenisChenu

DenisChenu

2022-05-24 17:01

developer   ~70015

About mantis:

new -> confirmed -> assigned -> quick review -> (testing if needed) -> (review if needed) -> merged -> closed

(testing if needed) -> (review if needed) can be
(testing if needed) -> (review if needed) -> (testing if needed) -> (review if needed) -> (testing if needed) -> (review if needed) etc … if needed.

I think a quick review can be done before testing to check the complexity of the commit.
Some commit can be merged just with a good review
But some other need more test

DenisChenu

DenisChenu

2022-05-24 17:01

developer   ~70016

Ping Gabriel ;)

DenisChenu

DenisChenu

2022-05-24 17:06

developer   ~70017

ping @ollehar
ping @c_schmitz

I think
we can have some search filter
Review (no assignment) : https://bugs.limesurvey.org/search.php?project_id=12&status=60&handler_id=-2&sticky=on&sort=date_submitted%2Clast_updated&dir=DESC%2CDESC&hide_status=-2&match_type=0

And i think not assigned is more clear :
For example in https://bugs.limesurvey.org/search.php?project_id=12&status=65&sticky=on&sort=date_submitted%2Clast_updated&dir=DESC%2CDESC&hide_status=-2&match_type=0
All seems assigned , i don't take it.

I think assigned can be used for assign on current task :)

gabrieljenik

gabrieljenik

2022-05-24 21:13

manager   ~70020

Hi Denis!

I see the main difference in between what I said and you said is this
quick review -> (testing if needed) -> (review if needed) -> resolved...
vs
testing->review->resolved...

I would set the workflow based on WHO is doing WHAT.
After a fix is done, it will be ready for tester (or someone else) to test it or review it .
I believe that's the thing to be signalized: Fix is done and DEV is releasing.

If the tester is a developer who is only checking code and will be doing a quick review, I am not sure that's something that need to be shown in the workflow, is it?
If the tester is doing one or many reviews, again, I am not sure it is something to be shown on the workflow.

Once the tester grabs it, I believe the tester should assign it to himself, and setting it to "review".

What do you think?

c_schmitz

c_schmitz

2022-05-25 13:09

administrator   ~70036

Last edited: 2022-05-25 13:57

My suggestion:

  • new -> Issue is new
  • confirmed -> Someone looked at it and was able to reproduce - otherwise ask reporter and set to 'Feedback'
  • assigned -> Assigned to a developer
  • 'Ready for code review' -> Development is done, ready for code review
  • 'In code review' -> if done move to 'ready for testing', otherwise move back to 'Assigned'
  • 'Ready for testing' -> ready for testing
  • 'In testing' -> Being tested. If successful move to 'To be merged' otherwise to 'Assigned'
  • 'To be merged' -> Ready for Merge - after merge move to resolved
  • 'Resolved' -> Merged to master but not released
  • 'Closed' -> Released

Additional rules:
Code review and/or testing can be omitted if the change is obviously uncritical and affects only 1-2 lines of code.

Let me know what you think...

DenisChenu

DenisChenu

2022-05-25 14:53

developer   ~70039

I would set the workflow based on WHO is doing WHAT.

Yes : Assign to is used for this. no ?

Once the tester grabs it, I believe the tester should assign it to himself, and setting it to "review".

Yes, then : more clear is it's (for example) : status = testing, but unassigned : https://bugs.limesurvey.org/permalink_page.php?url=https%3A%2F%2Fbugs.limesurvey.org%2Fsearch.php%3Fproject_id%3D4%26handler_id%3D-2%26sticky%3Don%26sort%3Dlast_updated%26dir%3DDESC%26hide_status%3D90%26match_type%3D0&permalink_token=20220525LaFn3Uarcox17SXTBt2CQQH_sbckYXtQ

@c_schmitz : you think review must be done before testing ?
Else : it's more for usage of mantis state :)

gabrieljenik

gabrieljenik

2022-05-25 15:32

manager   ~70040

@c_schmitz Does that apply to current options?
Is Mantis configured like that already?

image.png (7,400 bytes)   
image.png (7,400 bytes)   
c_schmitz

c_schmitz

2022-05-25 15:35

administrator   ~70041

Well I can extend the status list as needed. Just we need to agree first.

gabrieljenik

gabrieljenik

2022-05-25 16:05

manager   ~70042

OK. I would just do code review after testing.

gabrieljenik

gabrieljenik

2022-05-25 16:33

manager   ~70044

Another question.
After a DEV set it to "ready for testing", who would assign that to a tester?

DenisChenu

DenisChenu

2022-05-25 16:37

developer   ~70046

testing -> review
If issue : can be testing -> review again sometimes.
Reviewer can choose if need testing again or ok to merge after :)

After a DEV set it to "ready for testing", who would assign that to a tester?

My opinion: tester get it (assign it), dev unassign :)
Then : we don't need "Ready for testing"and "Ready for review"

Ready for testing == (status == testing && Unassigned)
In testing == (status == testing && assigned)

gabrieljenik

gabrieljenik

2022-05-25 16:38

manager   ~70047

At last, as we are speaking about Mantis, on the email notifications, can we maybe make more explicit who is doing the status changes?

Example. I get this line:

The following issue has been set as DUPLICATE OF issue 18074.

Would be great (easier and time saver) to see somehting like

The following issue has been set as DUPLICATE OF issue 18074, by C_SCHMITZ

c_schmitz

c_schmitz

2022-06-01 09:26

administrator   ~70157

Last edited: 2022-06-01 09:51

Internally we always do review before testing. Because we assume that code changes might introduce more bugs.
So, please do code review before testing.

c_schmitz

c_schmitz

2022-06-01 09:53

administrator   ~70158

I added the new statuses now. Let me know if there are any open questions.

DenisChenu

DenisChenu

2022-06-01 09:54

developer   ~70159

Then : we use "Ready for …" without updating assignment ?

gabrieljenik

gabrieljenik

2022-06-01 14:15

manager   ~70189

I would also keep "acknolwedged".
Bugs are created as "new".
Someone glance them but can't confirm them. Then they are set as "acknolwedged".

So people which need to confirm issues shall pick up status "acknolwedged" as well as new.
Moving them to "acknolwedged" is good for monitoring incoming bugs.

c_schmitz

c_schmitz

2022-06-02 14:12

administrator   ~70206

yeah, I did not touch acknowledged or confirmed.

@DenisChenu: Yea, if you use "Ready for" I would set the assignee to (empty).

gabrieljenik

gabrieljenik

2022-06-02 15:28

manager   ~70209

Final Set of Status

  • 'new' -> Issue is new
  • 'Acknolwedged' -> Issue is received and has been seen. Pending to be confirmed.
  • 'confirmed' -> Someone looked at it and was able to reproduce - otherwise ask reporter and set to 'Feedback'
  • 'assigned' -> Assigned to a developer
  • 'Ready for code review' -> Development is done, ready for code review
  • 'In code review' -> if done move to 'ready for testing', otherwise move back to 'Assigned'
  • 'Ready for testing' -> ready for testing
  • 'In testing' -> Being tested. If successful move to 'To be merged' otherwise to 'Assigned'
  • 'To be merged' -> Ready for Merge - after merge move to resolved
  • 'Resolved' -> Merged to master but not released
  • 'Closed' -> Released

Additional Rules:

  • Code review and/or testing can be omitted if the change is obviously uncritical and affects only 1-2 lines of code.
  • If you use "Ready for" it is ok to set the assignee to empty.
  • Internally we always do review before testing. Because we assume that code changes might introduce more bugs.
    So, please do code review before testing.

Issue History

Date Modified Username Field Change
2022-05-24 16:54 DenisChenu New Issue
2022-05-24 17:01 DenisChenu Note Added: 70015
2022-05-24 17:01 DenisChenu Bug heat 0 => 2
2022-05-24 17:01 DenisChenu Assigned To => gabrieljenik
2022-05-24 17:01 DenisChenu Status new => assigned
2022-05-24 17:01 DenisChenu Note Added: 70016
2022-05-24 17:02 DenisChenu Assigned To gabrieljenik =>
2022-05-24 17:02 DenisChenu Status assigned => confirmed
2022-05-24 17:06 DenisChenu Note Added: 70017
2022-05-24 21:13 gabrieljenik Note Added: 70020
2022-05-24 21:13 gabrieljenik Bug heat 2 => 4
2022-05-25 13:09 c_schmitz Note Added: 70036
2022-05-25 13:09 c_schmitz Bug heat 4 => 6
2022-05-25 13:10 c_schmitz Note Edited: 70036
2022-05-25 13:53 c_schmitz Note Edited: 70036
2022-05-25 13:54 c_schmitz Note Edited: 70036
2022-05-25 13:54 c_schmitz Note Edited: 70036
2022-05-25 13:57 c_schmitz Note Edited: 70036
2022-05-25 13:57 c_schmitz Note Edited: 70036
2022-05-25 13:57 c_schmitz Assigned To => c_schmitz
2022-05-25 13:57 c_schmitz Status confirmed => feedback
2022-05-25 14:53 DenisChenu Note Added: 70039
2022-05-25 14:53 DenisChenu Status feedback => assigned
2022-05-25 15:32 gabrieljenik Note Added: 70040
2022-05-25 15:32 gabrieljenik File Added: image.png
2022-05-25 15:35 c_schmitz Note Added: 70041
2022-05-25 16:05 gabrieljenik Note Added: 70042
2022-05-25 16:33 gabrieljenik Note Added: 70044
2022-05-25 16:37 DenisChenu Note Added: 70046
2022-05-25 16:38 gabrieljenik Note Added: 70047
2022-06-01 09:26 c_schmitz Note Added: 70157
2022-06-01 09:51 c_schmitz Note Edited: 70157
2022-06-01 09:53 c_schmitz Note Added: 70158
2022-06-01 09:54 DenisChenu Note Added: 70159
2022-06-01 10:11 c_schmitz Assigned To c_schmitz =>
2022-06-01 10:11 c_schmitz Status assigned => ready for testing
2022-06-01 14:15 gabrieljenik Note Added: 70189
2022-06-02 14:12 c_schmitz Note Added: 70206
2022-06-02 14:12 c_schmitz Assigned To => c_schmitz
2022-06-02 14:12 c_schmitz Status ready for testing => resolved
2022-06-02 14:12 c_schmitz Resolution open => fixed
2022-06-02 15:28 gabrieljenik Note Added: 70209