CommentFileSizeAuthor
#16 2466253-13.flag_.unflag-object-flag-counts.patch2.89 KBmartin107
#16 2466253-13.flag_.unflag-object-flag-counts-tests-only.patch2.31 KBmartin107
#13 2466253-13.flag_.unflag-object-flag-counts.patch2.89 KBjoachim
#13 2466253-13.flag_.unflag-object-flag-counts-tests-only.patch2.31 KBjoachim
#9 2466253-9.flag_.unflag-object-flag-counts.patch2.89 KBjoachim
#9 2466253-9.flag_.unflag-object-flag-counts-tests-only.patch2.31 KBjoachim
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

martin107’s picture

martin107 CreditAttribution: martin107 commented

When I was looking at a patch for #2465865: Change FlagService::flag() and ::unflag() to use objects to discourage use of IDs

My take on this bug was that

unflagByObject and unflag would have to be merged some how

and as well as the flag count being an issue -- well the handling of $account was also inconsistent and the merge would have to deal with that too.

I am suddenly busy -- will be silent for a few days..:(

joachim’s picture

joachim CreditAttribution: joachim commented

I'd rather we fix this bug first, so the commit history records a clear bug fix, separately from a refactor. Makes git blame and bisect much easier.

joachim’s picture

joachim CreditAttribution: joachim commented
Assigned: Unassigned » joachim

Assigning myself to this one then!

Thanks for all your contributions recently -- hope to see you again in the issue queue in future! (Also, BTW, I think I saw you're in the UK. Drop on in #drupal-uk on IRC sometime! :)

joachim’s picture

joachim CreditAttribution: joachim commented
Issue tags: +Needs tests
joachim’s picture

joachim CreditAttribution: joachim commented

Uhoh. I have a bad feeling about this:

    $counts[$flag_name] = db_select('flag_counts', 'fc')
      ->fields('fc', array('flagging_id'))
      ->condition('fid', $flag->fid)

Flags don't have a fid and haven't for ages. I suspect all our flag count API functions are broken!

joachim’s picture

joachim CreditAttribution: joachim commented
Status: Active » Postponed

Postponed by #2466385: flag count API functions are broken.

socketwench’s picture

socketwench CreditAttribution: socketwench commented
Related issues: +#2466423: Flag counts lacks test coverage

Looks like we missed adding test coverage for this too. >_<

joachim’s picture

joachim CreditAttribution: joachim commented
Status: Postponed » Active

We can't commit broken tests, so we need to fix this here, and add basic tests here, which we can then refine when the flag count API is fixed.

joachim’s picture

joachim CreditAttribution: joachim commented
Status: Active » Needs review
FileSize
2466253-9.flag_.unflag-object-flag-counts-tests-only.patch2.31 KB
2466253-9.flag_.unflag-object-flag-counts.patch2.89 KB

The last submitted patch, 9: 2466253-9.flag_.unflag-object-flag-counts-tests-only.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 9: 2466253-9.flag_.unflag-object-flag-counts.patch, failed testing.

joachim’s picture

joachim CreditAttribution: joachim commented

Weird. The patch fixes the problem for me when I try it by hand.

joachim’s picture

joachim CreditAttribution: joachim commented
FileSize
2466253-13.flag_.unflag-object-flag-counts-tests-only.patch2.31 KB
2466253-13.flag_.unflag-object-flag-counts.patch2.89 KB

Sigh...

-    $this->assertTrue($flag_count_flagged = $flag_count_pre + 1, "The flag count was incremented.");
+    $this->assertEqual($flag_count_flagged, $flag_count_pre + 1, "The flag count was incremented.");

I guess one reason we have assertEqual() is that it means you can't make the mistake I made of using = instead of ==.

joachim’s picture

joachim CreditAttribution: joachim commented
Status: Needs work » Needs review
joachim’s picture

joachim CreditAttribution: joachim commented

Argh, how do I trigger the testbot?

martin107’s picture

martin107 CreditAttribution: martin107 commented
FileSize
2466253-13.flag_.unflag-object-flag-counts-tests-only.patch2.31 KB
2466253-13.flag_.unflag-object-flag-counts.patch2.89 KB

Sometimes the only way is to re-upload. let see if this takes.

joachim’s picture

joachim CreditAttribution: joachim commented

Turns out it's not just us: #2467621: Tests not being added to the testbot queue.

The last submitted patch, 16: 2466253-13.flag_.unflag-object-flag-counts-tests-only.patch, failed testing.

The last submitted patch, 13: 2466253-13.flag_.unflag-object-flag-counts-tests-only.patch, failed testing.

martin107’s picture

martin107 CreditAttribution: martin107 commented
Issue tags: -Needs tests

Yes when the problem is laid bare.... it is obvious decrementFlagCounts() in the wrong place...

Tests look good and complete to me :)

So +1.

[Also This is a blocker so it would be good to get in. ]

joachim’s picture

joachim CreditAttribution: joachim commented
Status: Needs review » Reviewed & tested by the community

I'm going to call that an RTBC and commit it. Thanks for the review!

joachim’s picture

joachim CreditAttribution: joachim commented
Status: Reviewed & tested by the community » Fixed

  • joachim committed b633621 on 8.x-4.x 
    Issue #2466253 by joachim: Fixed FlagService::unflagByObject() failing...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.