Permalink
Browse files

Undo the change made in 5ce8563 because it messes up tag counts

(and makes the test fail-- I should have run that!).

Also, use Tag_Model::items() in save() to avoid code duplication.  Follow-on for #1628.
  • Loading branch information...
1 parent d835c06 commit 67d2e8081c6e5f0b679881bca3fdc81fe1e78ccc @bharat bharat committed Apr 23, 2011
Showing with 12 additions and 17 deletions.
  1. +12 −17 modules/tag/models/tag.php
View
@@ -69,36 +69,31 @@ public function items_count($type=null) {
* to this tag.
*/
public function save() {
- // Figure out what items have changed in this tag for our item_related_update event below
- if (isset($this->object_relations["items"])) {
- $added = array_diff($this->changed_relations["items"], $this->object_relations["items"]);
- $removed = array_diff($this->object_relations["items"], $this->changed_relations["items"]);
- if (isset($this->changed_relations["items"])) {
- $changed = array_merge($added, $removed);
- }
- $this->count = count($this->object_relations["items"]) + count($added) - count($removed);
- }
-
// Check to see if another tag exists with the same name
$duplicate_tag = ORM::factory("tag")
->where("name", "=", $this->name)
->where("id", "!=", $this->id)
->find();
if ($duplicate_tag->loaded()) {
- // If so, tag its items with this tag so as to merge it. Do this after we figure out what's
- // changed so that we don't notify on this change to keep churn down.
- $duplicate_tag_items = ORM::factory("item")
- ->join("items_tags", "items.id", "items_tags.item_id")
- ->where("items_tags.tag_id", "=", $duplicate_tag->id)
- ->find_all();
- foreach ($duplicate_tag_items as $item) {
+ // If so, tag its items with this tag so as to merge it.
+ foreach ($duplicate_tag->items() as $item) {
@alindeman

alindeman Apr 24, 2011

Contributor

I thought about using items() here too, but it scopes to viewable(). I suppose that will not be a problem because only admins can rename tags, but it seemed a bit fragile at the time.

@bharat

bharat Apr 24, 2011

Owner

I agree -- I think the fundamental problem is that viewable() is an application level concept, not model level. We should drop the viewable() from the model call and then find/fix all the app level places to call $tag->viewable()->items().

@alindeman

alindeman Apr 24, 2011

Contributor

Cool, I'll file a ticket about it.

$this->add($item);
}
// ... and remove the duplicate tag
$duplicate_tag->delete();
}
+ // Figure out what items have changed in this tag for our item_related_update event below
+ if (isset($this->object_relations["items"])) {
+ $added = array_diff($this->changed_relations["items"], $this->object_relations["items"]);
+ $removed = array_diff($this->object_relations["items"], $this->changed_relations["items"]);
+ if (isset($this->changed_relations["items"])) {
+ $changed = array_merge($added, $removed);
+ }
+ $this->count = count($this->object_relations["items"]) + count($added) - count($removed);
+ }
+
$result = parent::save();
if (!empty($changed)) {

2 comments on commit 67d2e80

Contributor

alindeman replied Apr 24, 2011

Haha, did this make my unit test fail? Hopefully.

Owner

bharat replied Apr 24, 2011

Yes, it did.. that's how I figured out that it was a problem! :-)

Please sign in to comment.