Closed Bug 1274371 Opened 9 years ago Closed 6 years ago

Store a single, current flag value, for each reviewer per commit

Categories

(MozReview Graveyard :: Review Board: Extension, defect, P1)

Product:
MozReview Graveyard
Component:
Review Board: Extension
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED INVALID

People

(Reporter: smacleod, Assigned: zalun)

References

Details

Attachments

(3 files, 1 obsolete file)

Store MozReview flags in a separate entity. (bug 1274371)
58 bytes, text/x-review-board-request
Details
mozreview: test storing MozReview flags in db. (bug 1274371)
59 bytes, text/x-review-board-request
Details
mozreview: create migrations file for reviewer flag in db (bug 1274371)
59 bytes, text/x-review-board-request
Details
After Bug 1197879 we will be storing a review flag value (r?, r-, r+, " ") on each review object of a review request. This makes it difficult to reason about the review flag value if we want to modify it without leaving a review (for example, if the author would like to re-request review). Moving forward we should store a single, current value, for each reviewer and tie this to the review request object (i.e. commit) rather than a review. The easiest place to store this data would be as part of CommitData.extra_data (We should probably still keep the flag value on review objects to show a history of flag change actions). As part of this, this flag store should have a ReviewRequestField which we can use to record changes into the ChangeDescription. This will allow us to render information about flag changes made by the author during publishing etc. This change will be required so that we can complete Bug 1195661 without introducing strange carryforward bugs.
Blocks: 1257229
Priority: -- → P1
This makes a quite complicated structure to write into a JSON field. Shouldn't we create a special database entity?
Assignee: nobody → pzalewa
Comment on attachment 8799339 [details] Store MozReview flags in a separate entity. (bug 1274371) https://reviewboard.mozilla.org/r/84540/#review91464 some comments after a quick reading of the code; this doesn't appear to be complete yet. i'll do a more complete review one these issues are addressed. ::: hgext/reviewboard/tests/test-reviewer-flags-in-db.t:1 (Diff revision 16) > +#require mozreviewdocker we rely on mirroring the flag state into extra_data to minimise changes to the rest of the code. all of these tests should also ensure that extra_data is correct. ::: hgext/reviewboard/tests/test-reviewer-flags.t:54 (Diff revision 16) > +Check if flag is created in the database > + i don't see this check happening here ::: pylib/mozreview/mozreview/managers.py:9 (Diff revision 16) > + I've been looking to a better place for this code, something > + like initial_data_extension. Initializing MozReviewExtension > + is too early in the django process to do that.""" this comment doesn't make sense; it looks more like a reminder to yourself ::: pylib/mozreview/mozreview/managers.py:19 (Diff revision 16) > + except ObjectDoesNotExist: > + return self.create( > + name='review', > + prefix='r', > + description='', > + values_json='["+", "-", "?", "X"]') as per our discussion about the "X" value, i don't think we should be storing deleted flags in the database. happy to can discuss this further with you can smac. if we keep it, the deleted status is represented by both "X" and " "; the same symbol should be used everywhere. note that "X" comes from the symbol you pass in to bugzilla to tell it to delete the flag, which results in the row being deleted from the database - it isn't a status that bugzilla uses beyond the api layer. ::: pylib/mozreview/mozreview/models.py:51 (Diff revision 16) > 'get_bugzilla_api_key', > 'get_or_create_bugzilla_users', > 'MozReviewUserProfile', > 'set_bugzilla_api_key', > 'UnverifiedBugzillaApiKey', > + 'MozReviewFlag', this list should be alphabetical. ::: pylib/mozreview/mozreview/models.py:102 (Diff revision 16) > + # Returns whether the flagtype is active or disabled. > + # TODO Consider removing it. Will there be a UI which is used to add flags > + # where one would choose a flag? > + is_active = models.BooleanField(default=True) looks like there's still work to do here. i vote for removing is_active. in the unlikely event we need to deactivate the review flag we can add support for it at that point. ::: pylib/mozreview/mozreview/models.py:108 (Diff revision 16) > + # TODO Consider removing it. Will there be a UI which is used to add flags > + # where one would choose a flag? > + is_active = models.BooleanField(default=True) > + # JSON string reprenting a list of allowed values ex. "['+', '-', '?', 'X']" > + values_json = models.CharField(max_length=200) > + # XXX Should we create a translation table for Bugzilla? looks like there's still work to do here. i don't know what this question means. ::: pylib/mozreview/mozreview/models.py:164 (Diff revision 16) > +I1. There might be multiple reviews per revision, but only the last one is > + important nit: only the last review from a single user is important. the way this is worded implies that if users A and B leave a review, only the newest review from either user is important, which isn't true. ::: pylib/mozreview/mozreview/models.py:172 (Diff revision 16) > +ToDo > +---- > +2. Add value validation - this should be done in a form > + > +QUESTIONS > +--------- > +As for I2 - Should all flags be cleared if a new revision is published? > +We're searching for new flags on published reviews. Isn't this covering this > +situation? > +STORE THE VALUE > + > +About validations: Do we want to move to Django form with reviews? > +raise Exception > + > +Should I always create a flag on publish, update timestamp or get_or_crete? > + > +ADD COMMENT to the flag type manager > + > +I thought it should be "Flag change not allowed, flag must be one of " (user > +is authorized, perfectly works with ' ') > + $ rbmanage create-review 6 --body-top "Cancel?" --public --review-flag='r%' > +- created review 2 > ++ API Error: 500: 225: Error publishing: Bugzilla error: You are not authorized to edit attachment 4 [details] [diff] [review]. > + > +ToDo > +---- > +glob: > +Check if clearing the reviewer list is supported > +X in bugzilla is removing from db - consider the same in rb looks like there's still work to do here. it isn't clear to me if these questions are reminders to yourself or questions to me. comments in code is kinda weird - it would be much better if you commented on the code in review board after pushing it up. ::: pylib/mozreview/mozreview/models.py:199 (Diff revision 16) > ++ API Error: 500: 225: Error publishing: Bugzilla error: You are not authorized to edit attachment 4 [details] [diff] [review]. > + > +ToDo > +---- > +glob: > +Check if clearing the reviewer list is supported yes, you can clear the reviewer list ::: pylib/mozreview/mozreview/resources/flag.py:84 (Diff revision 16) > + rr = ReviewRequest.objects.get(id=review_request_id) > + except ReviewRequest.DoesNotExist: > + return DOES_NOT_EXIST > + > + if not rr.is_accessible_by(request.user): > + logger.debug('rr not accessible') remove debugging code. as this is the only place the logger is used you should also remove the import & init ::: pylib/mozreview/mozreview/review_helpers.py:133 (Diff revision 16) > +def _old_get_reviewers_status(review_request, reviewers=None, > + include_drive_by=False): > + """Returns the latest review status for each reviewer. > + > + If include_drive_by is False, only reviewers nominated by the reviewee are > + considered. Set it to True to also report the status of `drive_by` > + reviewers. > + > + If a list of reviewers is provided, the returned dictionary will contain > + those reviewers regardless the value of include_drive_by > + """ > + > + logger.debug('%s %s %s' % ('!' * 30, 'CHECKING OLD CODE', '!' * 30)) > + designated_reviewers = review_request.target_people.all() > + if not reviewers: > + reviewers = designated_reviewers > + > + # We need to grab the reviews and statuses off the non-draft. > + if isinstance(review_request, ReviewRequestDraft): > + review_request = review_request.review_request > + > + reviewers_status = dict() delete prior versions of code instead of prepending _old ::: pylib/mozreview/mozreview/signal_handlers.py:661 (Diff revision 16) > # gets or creates the review and the second time it updates the > # object retrieved/created. This condition let us execute the code > # below only once. > > if not is_parent(review.review_request): > + logger.info(review.extra_data) remove debugging code ::: pylib/mozreview/mozreview/signal_handlers.py:664 (Diff revision 16) > + # Find latest review_request flag of that user and carry on > + # the status nit: we use "carry forward" not "carry on" elsewhere in the code ::: pylib/mozreview/mozreview/signal_handlers.py:679 (Diff revision 16) > + review.extra_data[REVIEW_FLAG_KEY] = status > + > + review.ship_it = (review.extra_data[REVIEW_FLAG_KEY] == 'r+') > + > + > +def _pre_save_review(sender, *args, **kwargs): this appears to be another old method which should be deleted
Attachment #8799339 - Flags: review?(glob) → review-
Comment on attachment 8799339 [details] Store MozReview flags in a separate entity. (bug 1274371) https://reviewboard.mozilla.org/r/84540/#review91464 > this comment doesn't make sense; it looks more like a reminder to yourself It's more like a note to the reviewer. I couldn't find an extension to add objects to the database, hence the chosen solution. It is working, although it has been criticized in a vidyo discussion.
Comment on attachment 8799339 [details] Store MozReview flags in a separate entity. (bug 1274371) https://reviewboard.mozilla.org/r/84540/#review91464 > looks like there's still work to do here. > > i vote for removing is_active. in the unlikely event we need to deactivate the review flag we can add support for it at that point. removed
Comment on attachment 8799339 [details] Store MozReview flags in a separate entity. (bug 1274371) https://reviewboard.mozilla.org/r/84540/#review91464 > It's more like a note to the reviewer. > I couldn't find an extension to add objects to the database, hence the chosen solution. > It is working, although it has been criticized in a vidyo discussion. i'm expecting the code that i'm reviewing to be in a state suitable for landing, so the code itself isn't the right place to put reviewer questions or notes. it's better to push the code up then use review board itself to add questions/comments to the appropriate lines of code. > as per our discussion about the "X" value, i don't think we should be storing deleted flags in the database. > > happy to can discuss this further with you can smac. > > if we keep it, the deleted status is represented by both "X" and " "; the same symbol should be used everywhere. > > note that "X" comes from the symbol you pass in to bugzilla to tell it to delete the flag, which results in the row being deleted from the database - it isn't a status that bugzilla uses beyond the api layer. i'll chat with smac and come to a decision here.
Comment on attachment 8799339 [details] Store MozReview flags in a separate entity. (bug 1274371) https://reviewboard.mozilla.org/r/84540/#review91464 > looks like there's still work to do here. > > it isn't clear to me if these questions are reminders to yourself or questions to me. > > comments in code is kinda weird - it would be much better if you commented on the code in review board after pushing it up. removed
Comment on attachment 8799339 [details] Store MozReview flags in a separate entity. (bug 1274371) https://reviewboard.mozilla.org/r/84540/#review91464 > i don't see this check happening here removed the message - all tests in `test-reviewer-flags-in-db.t`
Comment on attachment 8799339 [details] Store MozReview flags in a separate entity. (bug 1274371) https://reviewboard.mozilla.org/r/84540/#review91464 > i'm expecting the code that i'm reviewing to be in a state suitable for landing, so the code itself isn't the right place to put reviewer questions or notes. > > it's better to push the code up then use review board itself to add questions/comments to the appropriate lines of code. Message removed
Comment on attachment 8799339 [details] Store MozReview flags in a separate entity. (bug 1274371) https://reviewboard.mozilla.org/r/84540/#review91464 > i'll chat with smac and come to a decision here. as per our irc discussion let's remove all historical flag state tracking, and leave that in the hands of bugzilla (which already provides that feature, and it's already used as a source for reports). please work on updating the code so we only have a single row for the state. when a reviewer is removed, remove the row from the db.
Comment on attachment 8799339 [details] Store MozReview flags in a separate entity. (bug 1274371) https://reviewboard.mozilla.org/r/84540/#review91464 > as per our irc discussion let's remove all historical flag state tracking, and leave that in the hands of bugzilla (which already provides that feature, and it's already used as a source for reports). > please work on updating the code so we only have a single row for the state. when a reviewer is removed, remove the row from the db. Is this the desired logic? 1. On flag creation * Delete all flags (should be up to one) found by `review_request_id` and `requestee_id` * Do not create the flag object if it's status is "cleared" 2. On determining the status of a review * It is either the found flag's status (by `review_id`) or "cleared" (this happens when requestee is removed along with the flag). 3. On determining the status of a requestee * It is the found flag's status (by `requestee_id`) or "cleared" if requestee is in review request `target_people` list and has no flag.
Comment on attachment 8799339 [details] Store MozReview flags in a separate entity. (bug 1274371) https://reviewboard.mozilla.org/r/84540/#review91464 > Is this the desired logic? > > 1. On flag creation > * Delete all flags (should be up to one) found by `review_request_id` and `requestee_id` > * Do not create the flag object if it's status is "cleared" > 2. On determining the status of a review > * It is either the found flag's status (by `review_id`) or "cleared" (this happens when > requestee is removed along with the flag). > 3. On determining the status of a requestee > * It is the found flag's status (by `requestee_id`) or "cleared" if requestee is in > review request `target_people` list and has no flag. flag creation: there's no need to delete then recreate a flag. if the flag is being cleared, delete the existing row otherwise update the status of the current row in both cases i agree the key should be rr_id+requestee_id for the other two questions, i don't understand what you mean exactly by "status of a review" vs "status of a requestee" can you explain these a bit more, including where you'd use them.
Comment on attachment 8799339 [details] Store MozReview flags in a separate entity. (bug 1274371) https://reviewboard.mozilla.org/r/84540/#review91464 > flag creation: > > there's no need to delete then recreate a flag. > if the flag is being cleared, delete the existing row > otherwise update the status of the current row > in both cases i agree the key should be rr_id+requestee_id > > for the other two questions, i don't understand what you mean exactly by "status of a review" vs "status of a requestee" > can you explain these a bit more, including where you'd use them. You're certainly right about updating. As for the status reading. There are situations were status needs to be presented, but no flag is present. We display "(cleared)" next to the requestee after (s)he is cleared from reviewing the review request. Since the row is deleted we need a way to read that status somehow. Review might have no related flag. Status becomes "r?" The same for the requestee in a given rr. Cleared requestee remains in target_people, but has no flag. Status becomes "cleared"
Comment on attachment 8799339 [details] Store MozReview flags in a separate entity. (bug 1274371) https://reviewboard.mozilla.org/r/84540/#review91464 > You're certainly right about updating. > > As for the status reading. There are situations were status needs to be presented, but no flag is present. > > We display "(cleared)" next to the requestee after (s)he is cleared from reviewing the review request. > Since the row is deleted we need a way to read that status somehow. > > Review might have no related flag. Status becomes "r?" > > The same for the requestee in a given rr. Cleared requestee remains in target_people, but has no flag. Status becomes "cleared" i wonder if part of the problem here is we have extra states that are both not in bugzilla, and imho not clear as to their meaning. the states i see are: 1. requestee in target_people, flag r? 2. requestee in target_people, flag r+ 3. requestee in target_people, flag r- 4. requestee in target_people, no flag set 5. requestee not in target_people, flag r? 6. requestee not in target_people, flag r+ 7. requestee not in target_people, flag r- of these only the first three map directly to bugzilla, and have clear meanings: it's waiting for a review from someone, is good to land, or isn't good to land. in bugzilla clearing a flag removes you from the reviewers list (as it's a list of active flags). the last three are bug 1285855; the fix for that is to add users to the target_people list when they leave impromptu reviews. this leaves #4: requestee in target_people, no flag set. this state worries me - what does it actually mean? how would the patch author or reviewer know what the review outcomes or next steps are for a review request in this state? after discussions with smacleod and mcote i think the way forward here is to: - always create a flag-state row for each reviewer in target_people, defaulting to r? - remove the ability for a flag-state to be cleared (ui and backend) - i should fix bug 1285855 (but this shouldn't block the work here)
Comment on attachment 8799339 [details] Store MozReview flags in a separate entity. (bug 1274371) https://reviewboard.mozilla.org/r/84540/#review91464 > i wonder if part of the problem here is we have extra states that are both not in bugzilla, and imho not clear as to their meaning. > > the states i see are: > 1. requestee in target_people, flag r? > 2. requestee in target_people, flag r+ > 3. requestee in target_people, flag r- > 4. requestee in target_people, no flag set > 5. requestee not in target_people, flag r? > 6. requestee not in target_people, flag r+ > 7. requestee not in target_people, flag r- > > of these only the first three map directly to bugzilla, and have clear meanings: it's waiting for a review from someone, is good to land, or isn't good to land. in bugzilla clearing a flag removes you from the reviewers list (as it's a list of active flags). > > the last three are bug 1285855; the fix for that is to add users to the target_people list when they leave impromptu reviews. > > this leaves #4: requestee in target_people, no flag set. > this state worries me - what does it actually mean? how would the patch author or reviewer know what the review outcomes or next steps are for a review request in this state? > > after discussions with smacleod and mcote i think the way forward here is to: > - always create a flag-state row for each reviewer in target_people, defaulting to r? > - remove the ability for a flag-state to be cleared (ui and backend) > - i should fix bug 1285855 (but this shouldn't block the work here) as per our discussion: impromptu reviews will need special handling, as people need to be able to leave reviews without setting the flag state. we'd need to show the "clear" option in the flags menu to users who are not listed in target_people. we'll also need to remove the "?" state as it doesn't make sense for impromptu reviews. creation of flag-state rows for existing open review requests should be performed by a one-time migration script. (we spoke about a one-time script, lazy migration, or mapping missing-row to r?; one-time migration seems to be the least effort and provides the best data consistency).
Comment on attachment 8799339 [details] Store MozReview flags in a separate entity. (bug 1274371) https://reviewboard.mozilla.org/r/84540/#review91464 > as per our discussion: > > impromptu reviews will need special handling, as people need to be able to leave reviews without setting the flag state. we'd need to show the "clear" option in the flags menu to users who are not listed in target_people. we'll also need to remove the "?" state as it doesn't make sense for impromptu reviews. > > creation of flag-state rows for existing open review requests should be performed by a one-time migration script. (we spoke about a one-time script, lazy migration, or mapping missing-row to r?; one-time migration seems to be the least effort and provides the best data consistency). Here is how I understand it: 1. Requestee with a flag has to be in target_people * If requestee is in target_people, has to have a flag (it's created when added to target_people). * If impropmptu requestee sets a flag on the review, becomes a member of target_people. 2. When requestee is removed from target_people: * His flag is deleted from database if it's not related to review. * *Question*: What to do if a flag is related to a review? I think the requestee should be removed from the flag - setter would remain. 3. When requestee is setting a flag on review his flag is updated (review added if needed, status and timestamp changed). 4. There are different sets of possible statuses displayed in the drop-down: * Requestee from target_people: ['r?', 'r+', 'r-']. * For impromptu reviews: ['', 'r+', 'r-'], where '' implies no flag is gonna be created. There are some outcomes of these: 1. There is no ability to remove from target_people via setting the flag in review. 2. There are reviews without related flag. 3. Rows are unique by the [review_request, requestee] pair. 4. Neither UI nor backend has (cleared) status. It is simply not present in MozReview.
Clarification for point 2: the review object will stay, but the corresponding row should be removed from the flags table in all cases. The review, if it exists, will serve as history only. The rest sounds right to me.
(In reply to Piotr Zalewa [:zalun] from comment #31) > 4. Neither UI nor backend has (cleared) status. It is simply not present in > MozReview. someone who isn't in the target_people list needs to be able to leave comments on code, without setting the review flag. this is easily achieved by adding a clear option to the flag select which is selected by default. this clear option should never be presented to reviewers in the current target_people list.
Comment on attachment 8799339 [details] Store MozReview flags in a separate entity. (bug 1274371) https://reviewboard.mozilla.org/r/84540/#review91464 > Here is how I understand it: > > 1. Requestee with a flag has to be in target_people > * If requestee is in target_people, has to have a flag (it's created when added to target_people). > * If impropmptu requestee sets a flag on the review, becomes a member of target_people. > 2. When requestee is removed from target_people: > * His flag is deleted from database if it's not related to review. > * *Question*: What to do if a flag is related to a review? I think the requestee should be removed from the flag - setter would remain. > 3. When requestee is setting a flag on review his flag is updated (review added if needed, status and timestamp changed). > 4. There are different sets of possible statuses displayed in the drop-down: > * Requestee from target_people: ['r?', 'r+', 'r-']. > * For impromptu reviews: ['', 'r+', 'r-'], where '' implies no flag is gonna be created. > > There are some outcomes of these: > > 1. There is no ability to remove from target_people via setting the flag in review. > 2. There are reviews without related flag. > 3. Rows are unique by the [review_request, requestee] pair. > 4. Neither UI nor backend has (cleared) status. It is simply not present in MozReview. Question: Since publishing an impromptu review with a flag set should add the reviewer to `target_people`, review request should be published after this. Am I right?
Comment on attachment 8799339 [details] Store MozReview flags in a separate entity. (bug 1274371) https://reviewboard.mozilla.org/r/84540/#review91464 > Question: > Since publishing an impromptu review with a flag set should add the reviewer to `target_people`, review request should be published after this. > Am I right? that's really a question for bug 1285855. it shouldn't require a second publish - the target_people list should be updated within the same transaction/publish in which the flag was set.
how do i perform a migration of my existing database? in comment 30 we spoke about a one-off migration script, but i don't see anything in the latest revision that looks like that.
Flags: needinfo?(pzalewa)
It's not ready yet, so I haven't included in the review request.
Flags: needinfo?(pzalewa)
Depends on: 1320070
(In reply to Piotr Zalewa [:zalun] from comment #42) > It's not ready yet, so I haven't included in the review request. ah, that makes sense. is there any other work that isn't yet part of the patch? that would help scope my review.
All except of the migration part is there.
Comment on attachment 8799339 [details] Store MozReview flags in a separate entity. (bug 1274371) https://reviewboard.mozilla.org/r/84540/#review108256 there's a lot here, so here's a preliminary review. i performed some simple state changes with the ui, all as the same user. found the following issues. will perform further manual logic tests with the next revision. - as level3 set r?level3 -> fails with internal server error (value ? is not allowed for user level3) - published a review without changing flag, and without a comment -> created a "ship it" in rb that looked like a change, but wasn't -> created a useless comment in bugzilla that just contained the review url - deleted reviewer with r? set -> flag was not cleared in bugzilla ::: hgext/reviewboard/tests/test-bugzilla-review-interaction.t:973 (Diff revision 18) > + - id: 8 > + name: review > + requestee: reviewer2@example.com > + setter: author@example.com > + status: '?' this doesn't look right. clearing a flag should remove the flag from bugzilla ::: pylib/mozreview/mozreview/models.py:97 (Diff revision 18) > +class MozReviewFlagType(models.Model): > + """Flag Types to be used with class::MozReviewFlag""" > + # i.e. "review" > + name = models.CharField(max_length=200, unique=True) > + # prefix is used for presentation only i.e. 'r' > + prefix = models.CharField(max_length=4) 'prefix' is an odd name choice, and it isn't immediately obvious what it is. please change to 'short_name' ::: pylib/mozreview/mozreview/models.py:154 (Diff revision 18) > + def status(self): > + """Returns status of the flag including its type prefix.""" > + return '%s%s' % (self.type.prefix, self.value) this concerns me because the 'prefix/short_name' is for presentation only, but the name of this method doesn't reflect that. if status is later used in business logic, things will break if we change the "presentation only" prefix/short_name. let's remove this method completely, so callers need to be explicit in the data they are requesting. ::: pylib/mozreview/mozreview/models.py:176 (Diff revision 18) > + > + It is needed to be a class method as we validate the flag value in > + signal_handlers""" > + if value not in type.values: > + raise ValidationError("Value '%s' is not allowed." % value) > + remove trailing whitespace ::: pylib/mozreview/mozreview/models.py:190 (Diff revision 18) > + MozReviewFlag.validate_value(self.value, self.type, self.review, > + self.requestee, > + self.review_request.target_people.all()) > + > + > + def save(self, *args, **kwargs): pep8 nit: should only be one blank line between defs. ::: pylib/mozreview/mozreview/resources/flag.py:17 (Diff revision 18) > +) > +from mozreview.models import ( > + MozReviewFlag, > +) > + > +class MozReviewFlagResource(WebAPIResource): pep8: 2 blank lines before classes ::: pylib/mozreview/mozreview/resources/flag.py:33 (Diff revision 18) > + > + def _serialize(self, flag): > + return { > + 'id': flag.pk, > + 'type': flag.type_name, > + 'status': flag.status, remove 'status', and add 'value'. no need to flag_type.short_name as the full name is already returned as 'type' ::: pylib/mozreview/mozreview/review_helpers.py:111 (Diff revision 18) > > - for review in gen_latest_reviews(review_request): > - review_flag = review.extra_data.get(REVIEW_FLAG_KEY) > - user = review.user.username > > + # Find flags for each review pep8: too many blank lines ::: pylib/mozreview/mozreview/review_helpers.py:123 (Diff revision 18) > reviewers_status[user] = {} > > if user in reviewers_status: > - reviewers_status[user]['ship_it'] = review.ship_it > - if review_flag: > - reviewers_status[user]['review_flag'] = review_flag > + reviewers_status[user] = { > + 'ship_it': review_flag.review.ship_it, > + 'review_flag': review_flag.status} replace status with flagtype.short_name + flag.value ::: pylib/mozreview/mozreview/signal_handlers.py:225 (Diff revision 18) > +def manage_flags_on_rr_publishing(review_request, review_request_draft, > + user, has_diffset=False): > + # Manage flags > + # Flag of a requestee removed from target_people needs to be either > + # removed or requestee removed from the flag. > + if not is_parent(review_request): return instead of wrapping entire def in an if block ::: pylib/mozreview/mozreview/signal_handlers.py (Diff revision 18) > # locally, we handle it here, and log. > if not review_request_draft: > logger.error('Strangely, there was no review request draft on the ' > 'review request we were attempting to publish.') > return > - this blank line was nice :( ::: pylib/mozreview/mozreview/signal_handlers.py:348 (Diff revision 18) > raise CommitPublishProhibited() > > # The reviewid passed through p2rb is, for Mozilla's instance anyway, > # bz://<bug id>/<irc nick>. > reviewid = commit_data.draft_extra_data.get(IDENTIFIER_KEY, None) > + #import pdb; pdb.set_trace() remove debugging code ::: testing/vcttesting/reviewboard/mach_commands.py:47 (Diff revision 18) > > +def serialize_flag(flag): > + d = OrderedDict() > + d['id'] = flag['id'] > + d['type'] = flag['type'] > + d['status'] = flag['status'] needs updating following flag.status changes
Attachment #8799339 - Flags: review?(glob) → review-
Comment on attachment 8799339 [details] Store MozReview flags in a separate entity. (bug 1274371) https://reviewboard.mozilla.org/r/84540/#review108256 > this concerns me because the 'prefix/short_name' is for presentation only, but the name of this method doesn't reflect that. > > if status is later used in business logic, things will break if we change the "presentation only" prefix/short_name. > > let's remove this method completely, so callers need to be explicit in the data they are requesting. I think it would make a good __str__ function
Comment on attachment 8799339 [details] Store MozReview flags in a separate entity. (bug 1274371) https://reviewboard.mozilla.org/r/84540/#review108256 > I think it would make a good __str__ function i would prefer an explicitly name method. this makes it clear what the method returns.
Comment on attachment 8799339 [details] Store MozReview flags in a separate entity. (bug 1274371) https://reviewboard.mozilla.org/r/84540/#review108256 > this doesn't look right. clearing a flag should remove the flag from bugzilla I've fixed it, but we agreed that sending an ' ' flag is not possible from within UI
Attachment #8828354 - Attachment is obsolete: true
Attachment #8828354 - Flags: review?(glob)
Comment on attachment 8799339 [details] Store MozReview flags in a separate entity. (bug 1274371) https://reviewboard.mozilla.org/r/84540/#review117420 had a quick skim and saw you used __str__; bouncing back to you to fix before going further. ::: pylib/mozreview/mozreview/models.py:192 (Diff revision 19) > + def __str__(self): > + return '%s%s' % (self.type.short_name, self.value) as per my response when you asked if we should use __str__ - don't use __str__.
Attachment #8799339 - Flags: review?(glob) → review-
Attachment #8843180 - Flags: review?(glob)
Blocks: 1199007
Comment on attachment 8799339 [details] Store MozReview flags in a separate entity. (bug 1274371) this work is on hold; clearing reviews
Attachment #8799339 - Flags: review?(glob)
Attachment #8843179 - Flags: review?(glob)
Attachment #8843180 - Flags: review?(glob)
MozReview is now obsolete. Please use Phabricator instead. Closing this bug.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: