Jump to content


Review status changes back to "Needs Review" after commit - Swarm 2014.3

2014.3 swarm review status

  • Please log in to reply
19 replies to this topic

#1 jspang

jspang

    Member

  • Members
  • PipPip
  • 26 posts

Posted 21 August 2014 - 09:33 PM

We just recently upgraded to Swarm 2014.3 (SWARM/2014.3/897280 (2014/07/24)), and are starting to notice this behavior. Here are some repro steps that work for me:
  • Check out a file, edit it, then shelve with "#review" to start a Swarm review
  • Open Swarm and change the review status to "Approved"
  • In Perforce, submit the Changelist (you can either delete the shelved file to submit, or submit directly via 'p4 submit -e')
  • Refresh the review page in Swarm
After submitting the changelist, the Swarm review appears to go back to the 'Needs review' state. We noticed this behavior starting when we upgraded to 2014.3, but are not 100% confident that's when it started.

Is this a known issue? Are there any patches we can apply to fix it?

#2 P4PetrH

P4PetrH

    Advanced Member

  • Members
  • PipPipPip
  • 36 posts

Posted 22 August 2014 - 05:25 PM

Swarm automatically reverts review state from 'Approved' to 'Needs Review' when its updated with a new version (including commits) that is different from previous. By the term 'different' in this context we mean changes in file names and/or file types and/or file contents (digests), see http://www.perforce....ews.states.html (check 'Approved' paragraph under 'States' section).

So the behaviour you are describing should happen if the committed change is 'different' in terms described above compared to the latest review version. Are you sure you are committing unmodified files in this respect? Do you get the same problem if you submit the review from Swarm?

#3 jspang

jspang

    Member

  • Members
  • PipPip
  • 26 posts

Posted 22 August 2014 - 06:27 PM

I tried all three of these repro steps, and the problem happened with each of them:
  • Unshelving, modifying the file, then submitting
  • Unshelving, NOT modifying the file, then submitting
  • Submitting directly from the shelf (p4 submit -e)
I would have expected this behavior to happen with #1, but not #2 or #3. This did not repro on our older 2014.2 server.

As far as submitting from within Swarm, we have the 'disable commit' configuration set to True, so I can't easily test that.

#4 P4PetrH

P4PetrH

    Advanced Member

  • Members
  • PipPipPip
  • 36 posts

Posted 22 August 2014 - 06:59 PM

We added this feature in 2014.3, so you can't repro it in 2014.2.

Lets focus on #2, it certainly should not revert the review state. Could you send me output of the following commands (you can send it via private message if you don't want to share it publicly):

1. p4 fstat -e <REVIEW_ID> -Ol -T "depotFile,headAction,headType,headRev,resolved,unresolved,digest" -Rs //...@=<REVIEW_ID>
2. p4 fstat -e <CHANGE_ID> -Ol -T "depotFile,headAction,headType,headRev,resolved,unresolved,digest" //...@=<CHANGE_ID>

where you replace <REVIEW_ID> with the id of the review you are having issue with and <CHANGE_ID> with submitted change id.

Thanks!

#5 jspang

jspang

    Member

  • Members
  • PipPip
  • 26 posts

Posted 22 August 2014 - 07:11 PM

The fstat of the review does not return anything because the review CL does not have any files in it... pending or shelved. I can send you the output from the CHANGE_ID but I have a feeling that's useless without the review to compare it to. :)

Perhaps the review deleted the shelved files after I deleted them from my Change? (in order to submit it). That would certainly explain why the digest is different.

#6 P4PetrH

P4PetrH

    Advanced Member

  • Members
  • PipPipPip
  • 36 posts

Posted 22 August 2014 - 09:21 PM

My apologies, I didn't say it explicitly, but you need to run the fstat in step 1 before you submit the change (ensure that no other versions are added to the review between steps 1 and 2). Swarm clears files from review-associated shelf when the review is submitted.

#7 jspang

jspang

    Member

  • Members
  • PipPip
  • 26 posts

Posted 25 August 2014 - 07:11 PM

Swarm is behaving... confusingly. When I try to repro this, it doesn't look like Swarm is saving its own copy of the shelved CL. Is there additional behavior I need to watch out for when trying to set up this repro?

Here's my original CL:
C:\>p4 describe -s -S 1426759
Change 1426759 by jspang@jspang_JSPANG-DEV on 2014/08/25 11:50:09 *pending*
		More Swarm testing.
		#review-1426760
Shelved files ...
... //depot/test/1.txt#57 edit
And here's the review CL (note the lack of shelved files):
C:\>p4 describe -s -S 1426760
Change 1426760 by swarmuser@swarm-10dd32b2-4eba-fc5f-e81e-aea30a098a64 on 2014/08/25 11:50:16 *pending*
		More Swarm testing.
Shelved files ...
Despite this, I can still see the contents of the file in Swarm. I don't understand why Swarm sometimes has a shelved copy of the file and sometimes doesn't. Please forgive me if this is a whole different topic, but I'm having problems getting the information you asked because of this!

#8 P4PetrH

P4PetrH

    Advanced Member

  • Members
  • PipPipPip
  • 36 posts

Posted 25 August 2014 - 10:47 PM

No problem, you should be able to see the files as long as the review is pending. Swarm clears files from a review-associated shelf (i.e. shelved change with the same id as review) when you update it with a commit. I assume you didn't commit the review, correct? When you open the review page in Swarm for review 1426760 and hover over the last (rightmost) version in versions slider, what does it say? Can you also run this command and send the output (it should be json-encoded data for review 1426760):

p4 key swarm-review-ffea3ab7

Thanks!

#9 jspang

jspang

    Member

  • Members
  • PipPip
  • 26 posts

Posted 25 August 2014 - 11:41 PM

The versions slider is grayed out (well, not interactable), so hovering over it does not display anything. However, here is the p4 key information you asked for!

{"type":null,"changes":[1426759],"commits":null,"versions":null,"author":"jspang","participants":{"jspang":[]},"hasReviewer":0,"description":"More Swarm testing.","created":1408992616,"updated":1408992616,"projects":null,"state":"needsReview","testStatus":null,"testDetails":null,"deployStatus":null,"deployDetails":null,"pending":1,"commitStatus":null,"token":"8742499C-0765-BD8A-1B78-E11E4AA1ED49","upgrade":4}


#10 P4PetrH

P4PetrH

    Advanced Member

  • Members
  • PipPipPip
  • 36 posts

Posted 26 August 2014 - 05:22 PM

Thanks for providing the output from p4 key command! It looks like your review is not updated from a change as your review has no versions (thats why your version slider has no 'nodes' to hover over). Normally, when you create or update a review, Swarm adds a version to it (you would also see it under the 'versions' key when running the 'p4 key swarm-review-xxxxx' command). To narrow it down, could you please answer following questions:

1. Do you see any related errors in log?

2. Do you have any triggers set in Perforce?

3. When you update review 1426760 again (i.e. in your case you just reshelve change 1426759 with modified //depot/test/1.txt file), does it add a new version to the review 1426760?

4. In Swarm, when you create/update review, do you see any associated activity (on the home page or under 'History' tab on the particular review page)?


Thanks for the time spending on this!

#11 jspang

jspang

    Member

  • Members
  • PipPip
  • 26 posts

Posted 08 September 2014 - 08:31 PM

Hey there, sorry about the delayed response. I've just communicated with my users about this, and we think we see why Swarm is changing the status back to 'Needs Approval'. I don't think there's any bug here.

I did get a follow up question: Is there any way to disable this functionality? Some of our devs would like review statuses to stay 'Approved' once they are changed so, and to not automatically change back. Is this possible?

#12 P4PetrH

P4PetrH

    Advanced Member

  • Members
  • PipPipPip
  • 36 posts

Posted 09 September 2014 - 12:40 AM

No problem, I am glad you figured it out! Regarding your follow-up question, there is no configuration option to disable reverting the review status when review is updated. The only way you can turn it off, as I can think of at the moment, is to modify the source code. Its easy, you just need to comment one line of code (details below), however be careful - you will have to remember doing it every time you upgrade Swarm. Also please note that it will disable this feature for ANY review in Swarm, I am not sure if you want this (based on you said that only SOME of your devs would like this feature turned off).

Here are the instructions:

1. Open <SWARM_ROOT>/module/Reviews/src/Reviews/Model/Review.php in your text editor
2. Search for "revert state back to 'Needs Review", you should find the following lines of code

		 // revert state back to 'Needs Review' if review was approved and the new version is different
		 if ($this->getState() === static::STATE_APPROVED && $changesDiffer === 1) {
			 $this->setState(static::STATE_NEEDS_REVIEW);
		 }

(if you are on 2014.3, these are lines 306-309)

3. Replace these lines with
		 // revert state back to 'Needs Review' if review was approved and the new version is different
		 if ($this->getState() === static::STATE_APPROVED && $changesDiffer === 1) {
			 // $this->setState(static::STATE_NEEDS_REVIEW);
		 }

4. Restart Apache to ensure workers will use the modified code.

If you use git reviews, you will also need to modify <SWARM_ROOT>/module/Reviews/src/Reviews/Model/GitReview.php in a similar way. Look for the same string, you should find

	 // revert state back to 'Needs Review' if review was approved and the new version is different
	 // we specifically check for 1 as that indicates a difference in a key field such as filenames, digests or type
	 // if the new version is pending it must be the authorative change and we have nothing to compare
	 // it to, so we assumed it changed
	 if ($this->getState() === static::STATE_APPROVED
		 && ($change->isPending() || $this->changesDiffer($this->getId(), $change) === 1)
	 ) {
		 $this->setState(static::STATE_NEEDS_REVIEW);
	 }

and replace it with

	 // revert state back to 'Needs Review' if review was approved and the new version is different
	 // we specifically check for 1 as that indicates a difference in a key field such as filenames, digests or type
	 // if the new version is pending it must be the authorative change and we have nothing to compare
	 // it to, so we assumed it changed
	 if ($this->getState() === static::STATE_APPROVED
		 && ($change->isPending() || $this->changesDiffer($this->getId(), $change) === 1)
	 ) {
		 // $this->setState(static::STATE_NEEDS_REVIEW);
	 }

Instead of commenting the line of code, you can also delete the line or the whole block with the same effect.

#13 jspang

jspang

    Member

  • Members
  • PipPip
  • 26 posts

Posted 09 September 2014 - 08:53 PM

Thanks a bunch! I'll follow up with our users to see if we REALLY want to disable this or not.

#14 Andrew DeFaria

Andrew DeFaria

    Advanced Member

  • Members
  • PipPipPip
  • 125 posts

Posted 13 March 2015 - 12:17 AM

Ugh! You should fix your forum software. If I type stuff here and then say click on jspang's name I go to his personal page but when I return here my comment is lost! Now what did I say again...

I agree with jspang's original assessment that if you submit the change list of an approved review using P4V then the review status should not change. I also see the point that if something in the changelist changed then you need to re-review it. I dont' think that anything should be considered changed if all that was done was to submit the changelist. But you seem to be saying that something did change. What changed?

(Dang I said it better the first time).

#15 Andrew DeFaria

Andrew DeFaria

    Advanced Member

  • Members
  • PipPipPip
  • 125 posts

Posted 18 March 2015 - 05:35 PM

To bring this up again, I found under Unapprove modified reviews:

By default, when an approved review is committed or updated, Swarm changes the state to Needs Review if the files have been modified since the review was approved.

If one or more files in a review has the filetype +k (ktext), this behavior is undesirable because the files will appear to be modified as the Perforce service replaces RCS keywords with their current values.

This behavior can be disabled. Edit the data/config.php file, and add or update the unapprove_modified item to false, within the reviews configuration block.


This seems to be an all or nothing type thing. I couldn't think of a good reason to change the state of the review to Needs Review if only RCS keywords had changed. Then upon re-reading the above I noticed the phrase "If one of more". This means that there could be a changelist with one file having RCS keywords replaced and one or more files who are not +k (ktext) who have been modified after the review. Is that true because if so then that's different.

I don't see, however, why the current "compare this file to the version in the review and tell me if it's been changed" can't become smart enough to also say "oh and if the only changes are replacement of RCS keywords on a +k (ktext) file then don't report that file as being modified".

Please advise...


#16 Andrew DeFaria

Andrew DeFaria

    Advanced Member

  • Members
  • PipPipPip
  • 125 posts

Posted 19 March 2015 - 04:44 PM

Another question. I hope this is configurable but it seems to me (actually my user asked this) that if Swarm is going to change a review's state like from Approved to Needs Review it oughta send out email to the reviewers (or at least the review author) that the state has been changed and a review needs to be conducted again. If there is not a way to configure this then consider it a request for enhancement and let me know if I need to request this enhancement someplace else or will this do?

Speaking of enhancements, is there any way to change the email contents when Swarm sends out email? I ask because they are rather bland (better than just plain text but not that exciting) and I've had many users confused between the "changed" email that it sends out and the "review" email that is sends out. Even I often have to re-read the email and say "Oh this isn't a review - it's just a change". I'm thinking say adding a large image that can server to easily and at a glance allow the user to differentiate between the two. And again, if this needs to be entered in some Request For Enhancement area let me know.

#17 P4Matt

P4Matt

    Advanced Member

  • Members
  • PipPipPip
  • 1383 posts

Posted 20 March 2015 - 05:41 AM

Generally Swarm sends email on every review state change. I know they were doing some work reduce the number of emails sent, so they may have cut  those out. I'll try to repro and get back to you.

Conveniently the docs actually cover modifying the email templates! (I love it when we stuff like that!) Here's a link to the appropriate docs:

https://swarm.worksh...mple_email.html

Feature requests are fine here by the way. We're always open to feedback on our work.

#18 P4Matt

P4Matt

    Advanced Member

  • Members
  • PipPipPip
  • 1383 posts

Posted 20 March 2015 - 05:48 AM

On your unapprove submit question, I think we could be smarter about it. I'll file an enhancement request on your behalf.

#19 haiderm

haiderm

    Newbie

  • Members
  • Pip
  • 7 posts

Posted 23 April 2015 - 06:09 PM

I have also noticed this behavior. It does not happen with every change, but sometimes, Swarm changes a review from "Approved" to "Needs Review" on commit. When inspecting the change in Swarm, using the diff version of the file view (the line instead of just the dot), Swarm says "No modified files." So, on one hand Swarm noticed some modification because it re-opened the review, and on the other it does not tell the user what the modification was. I can't see how that is desirable behavior.

#20 BrianH

BrianH

    Member

  • Members
  • PipPip
  • 15 posts
  • LocationMiddleton, WI

Posted 07 March 2017 - 05:42 PM

View PostP4Matt, on 20 March 2015 - 05:48 AM, said:

On your unapprove submit question, I think we could be smarter about it. I'll file an enhancement request on your behalf.

Jumping on this train as well. It seems like the correct behavior to not notify on a submit of the associated changelist.



Also tagged with one or more of these keywords: 2014.3, swarm, review status

0 user(s) are reading this topic

0 members, 0 guests, 0 anonymous users