{"action":"created","issue":{"url":"https://api.github.com/repos/sul-dlss/dor-services/issues/61","labels_url":"https://api.github.com/repos/sul-dlss/dor-services/issues/61/labels{/name}","comments_url":"https://api.github.com/repos/sul-dlss/dor-services/issues/61/comments","events_url":"https://api.github.com/repos/sul-dlss/dor-services/issues/61/events","html_url":"https://github.com/sul-dlss/dor-services/issues/61","id":77899444,"number":61,"title":"ContentMetadataDS has bogus and inconsistent methods, fake tests","user":{"login":"atz","id":109954,"avatar_url":"https://avatars.githubusercontent.com/u/109954?v=3","gravatar_id":"","url":"https://api.github.com/users/atz","html_url":"https://github.com/atz","followers_url":"https://api.github.com/users/atz/followers","following_url":"https://api.github.com/users/atz/following{/other_user}","gists_url":"https://api.github.com/users/atz/gists{/gist_id}","starred_url":"https://api.github.com/users/atz/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/atz/subscriptions","organizations_url":"https://api.github.com/users/atz/orgs","repos_url":"https://api.github.com/users/atz/repos","events_url":"https://api.github.com/users/atz/events{/privacy}","received_events_url":"https://api.github.com/users/atz/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":2,"created_at":"2015-05-19T02:37:38Z","updated_at":"2015-05-19T05:59:00Z","closed_at":null,"body":"Like this gem:\r\n```ruby\r\n    def move_resource resource_name, new_position\r\n      xml=self.ng_xml\r\n      file_node=xml.search('//resource[@id=\\''+resource_name+'\\']')\r\n      if(file_node.length!=1)\r\n        raise 'Resource not found or duplicate found.'\r\n      end\r\n      position=file_node.first['sequence'].to_i\r\n      #is the resource being moved earlier in the sequence or later?\r\n      new_position=new_position.to_i\r\n      if new_position>position\r\n        counter=position\r\n        while true\r\n          if counter == position\r\n            break\r\n          end\r\n          item=xml.search('/resource[@id=\\''+counter.to_s+'\\']').first\r\n          counter=counter+1\r\n          item['sequence']=counter.to_s\r\n        end\r\n      else\r\n        counter=position\r\n        while true\r\n          if counter == new_position\r\n            break\r\n          end\r\n          item=xml.search('/resource[@id=\\''+counter.to_s+'\\']').first\r\n          counter=counter-1\r\n          item['sequence']=counter.to_s\r\n        end\r\n      end\r\n    end\r\n```\r\n\r\nSo on `new_position > position`... do nothing?  Notably, the test for this method called it, but never defined any expectation about what it wanted to result.  Just don't crash = OK!\r\n\r\nAnother head scratcher:\r\n```ruby\r\n    def update_resource_type resource, new_type\r\n      xml=self.ng_xml\r\n      resource_node=xml.search('//resource[@id=\\''+resource_name+'\\']')\r\n      if(resource_node.length!=1)\r\n        raise 'Resource not found or duplicate found.'\r\n      end\r\n      resource_node.first['type']=new_type\r\n    end\r\n```\r\n\r\nThe method receives `resource` but then uses only `resource_name`.  In fact, it should crash right?  Turns out the test named for it also defines no expectation.... and doesn't even invoke it!  \r\n\r\nMore broadly, there is confusion about when modification code should:\r\n- write XML back to the datastream (`self.content=xml.to_s`)\r\n- `self.save`\r\n\r\nDoes a modifying method update XML and save?  Quick survey:\r\n- `add_file`: both\r\n- `add_resource`: both\r\n- `remove_resource`: both\r\n- `remove_file`: both\r\n- `update_attributes `: both\r\n- `update_file`: both \r\n- `rename_file`: both \r\n- `update_resource_label`: neither\r\n- `update_resource_type`:  neither (**invalid**)\r\n- `move_resource`: neither (**broken**)\r\n- `set_content_type`: sets XML only\r\n\r\n\r\n\r\nIn order to avoid 10,000 writes, I suspect the proper answer is that it should **never** call save automatically.  Don't write it until explicitly told.  After all, we are following an ActiveSupport pattern here.  But that entails making sure none of our consumers are relying on it.  "},"comment":{"url":"https://api.github.com/repos/sul-dlss/dor-services/issues/comments/103348962","html_url":"https://github.com/sul-dlss/dor-services/issues/61#issuecomment-103348962","issue_url":"https://api.github.com/repos/sul-dlss/dor-services/issues/61","id":103348962,"user":{"login":"jmartin-sul","id":7741604,"avatar_url":"https://avatars.githubusercontent.com/u/7741604?v=3","gravatar_id":"","url":"https://api.github.com/users/jmartin-sul","html_url":"https://github.com/jmartin-sul","followers_url":"https://api.github.com/users/jmartin-sul/followers","following_url":"https://api.github.com/users/jmartin-sul/following{/other_user}","gists_url":"https://api.github.com/users/jmartin-sul/gists{/gist_id}","starred_url":"https://api.github.com/users/jmartin-sul/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jmartin-sul/subscriptions","organizations_url":"https://api.github.com/users/jmartin-sul/orgs","repos_url":"https://api.github.com/users/jmartin-sul/repos","events_url":"https://api.github.com/users/jmartin-sul/events{/privacy}","received_events_url":"https://api.github.com/users/jmartin-sul/received_events","type":"User","site_admin":false},"created_at":"2015-05-19T05:58:59Z","updated_at":"2015-05-19T05:58:59Z","body":"my two cents:  if we can find all of the callers of all of those methods, i'd vote for changing the methods to never save.  if it's obvious from the code surrounding a call whether an implied save is expected, we can add or not add the save call there accordingly.  if it's not clear and it'd be difficult to figure out, it seems safe to just add a save call right after the modification call, because at worst you end up doing the same thing."}}