Repository: dag-hammarskjold-library/dlx

Issue:
Don't add 001 to seriazliations if there is no record ID
New and temporary records don't have record IDs. In serialization to MRK, a 001 field with a value of None is being added to these records. The field should not be added at all.

Discussion:

Solution patch:
diff --git a/dlx/marc/__init__.py b/dlx/marc/__init__.py
index 968aa7d1..7948e916 100644
--- a/dlx/marc/__init__.py
+++ b/dlx/marc/__init__.py
@@ -171,14 +171,12 @@ def from_table(cls, table, auth_control=True, auth_flag=False, field_check=None,
                 value = table.index[temp_id][field_name]
                 
                 # parse the column header into tag, code and place
-                if match := re.match(r'^(([1-9]+)\.)?(\d{3})(\$)?([a-z0-9])', str(field_name)):
+                if match := re.match(r'^(([1-9]+)\.)?(\d{3})(\$)?([a-z0-9])?', str(field_name)):
                     if match.group(1):
                         instance = int(match.group(2))
                         instance -= 1 # place numbers start at 1 in col headers instead of 0
 
                     tag, code = match.group(3), match.group(5)
-                elif len(field_name) == 3 and field_name[0:2] == '00':
-                    tag, code = field_name, None
                 else:
                     exceptions.append('Invalid column header "{}"'.format(field_name))
                     continue
@@ -187,6 +185,9 @@ def from_table(cls, table, auth_control=True, auth_flag=False, field_check=None,
                     exceptions.append('Column header {}.{}{} is repeated'.format(instance, tag, code))
                     continue
 
+                if tag == '001':
+                    record.id = int(value)
+
                 # if the subfield field is authority-controlled, use subfield 
                 # $0 as the xref. $0 will be the first subfield in the field if
                 # it exists, because the header has been sorted.
@@ -245,7 +246,7 @@ def from_excel(cls, path, auth_control=True, auth_flag=False, field_check=None,
         return cls.from_table(table, auth_control=auth_control, field_check=field_check)
 
     @classmethod
-    def from_xml_raw(cls, root, *, auth_control=False):
+    def from_xml_raw(cls, root, *, auth_control=False, delete_subfield_zero=True):
         assert isinstance(root, ElementTree.Element)
         self = cls()
         
@@ -253,13 +254,13 @@ def from_xml_raw(cls, root, *, auth_control=False):
 
         for r in root.findall('record'):
             i += 1
-            self.records.append(self.record_class.from_xml_raw(r, auth_control=auth_control))
+            self.records.append(self.record_class.from_xml_raw(r, auth_control=auth_control, delete_subfield_zero=delete_subfield_zero))
 
         return self
         
     @classmethod
-    def from_xml(cls, string, auth_control=False):
-        return cls.from_xml_raw(ElementTree.fromstring(string), auth_control=auth_control)
+    def from_xml(cls, string, auth_control=False, delete_subfield_zero=True):
+        return cls.from_xml_raw(ElementTree.fromstring(string), auth_control=auth_control, delete_subfield_zero=delete_subfield_zero)
 
     @classmethod
     def from_mrk(cls, string, *, auth_control=True):
@@ -353,7 +354,7 @@ def to_table(self, *, write_id=True) -> Table:
             # each record is one table row
             i += 1
             
-            if write_id:
+            if write_id and record.id is not None:
                 table.set(i, '1.001', str(record.id))
             elif field := record.get_field('001'):
                 table.set(i, '1.001', field.value)
@@ -1346,7 +1347,7 @@ def to_mij(self):
     def to_mrc(self, *tags, language=None, write_id=True):
         record = copy.deepcopy(self)
 
-        if write_id:
+        if write_id and record.id is not None:
             record.set('001', None, str(record.id))
 
         directory = ''
@@ -1388,7 +1389,7 @@ def to_mrc(self, *tags, language=None, write_id=True):
     def to_mrk(self, *tags, language=None, write_id=True):
         record = copy.deepcopy(self) # so as not to alter the original object's data
 
-        if write_id:
+        if write_id and record.id is not None:
             record.set('001', None, str(record.id))
         
         return '\n'.join([field.to_mrk(language=language) for field in record.get_fields()]) + '\n'
@@ -1416,7 +1417,7 @@ def to_str(self, *tags, language=None):
     def to_xml_raw(self, *tags, language=None, xref_prefix='', write_id=True):
         record = copy.deepcopy(self) # so as not to alter the orginal object's underlying data
 
-        if write_id:
+        if write_id and record is not None:
             record.set('001', None, str(record.id))
         
         # todo: reimplement with `xml.dom` or `lxml` to enable pretty-printing
@@ -1489,6 +1490,8 @@ def from_mij(self, string):
     @classmethod
     def from_mrc(cls, string):
         '''Parses a MARC21 string (.mrc) into dlx.Marc'''
+
+        raise Exception("This method is unfinished")
         
         self = cls()
         base = string[12:17]
@@ -1513,6 +1516,9 @@ def from_mrk(cls, string: str, auth_control=True, delete_subfield_zero=True):
 
             if tag[:2] == '00':
                 field = Controlfield(tag, rest)
+                
+                if tag == '001':
+                    self.id = int(field.value)
             else:
                 ind1, ind2 = [x.replace('\\', ' ') for x in rest[:2]]
                 field = Datafield(record_type=cls.record_type, tag=tag, ind1=ind1, ind2=ind2)
@@ -1573,7 +1579,13 @@ def from_xml_raw(cls, root: ElementTree.Element, *, auth_control=True, delete_su
         self = cls()
             
         for node in filter(lambda x: re.search('controlfield$', x.tag), root):
-            self.set(node.attrib['tag'], None, node.text)
+            tag, value = node.attrib['tag'], node.text
+            field = Controlfield(tag, value)
+
+            if tag == '001':
+                self.id = int(value)
+
+            self.fields.append(field)
 
         for field_node in filter(lambda x: re.search('datafield$', x.tag), root):
             tag = field_node.attrib['tag']
@@ -1619,8 +1631,8 @@ def from_xml_raw(cls, root: ElementTree.Element, *, auth_control=True, delete_su
         return self
         
     @classmethod
-    def from_xml(cls, string, auth_control=True):
-        return cls.from_xml_raw(ElementTree.fromstring(string), auth_control=auth_control)
+    def from_xml(cls, string, auth_control=True, delete_subfield_zero=True):
+        return cls.from_xml_raw(ElementTree.fromstring(string), auth_control=auth_control, delete_subfield_zero=delete_subfield_zero)
 
     @classmethod
     def from_json(cls, string, auth_control=False):


Test patch:
diff --git a/tests/test_marc.py b/tests/test_marc.py
index c9804572..5df686a4 100644
--- a/tests/test_marc.py
+++ b/tests/test_marc.py
@@ -726,6 +726,9 @@ def test_to_xml(db):
     control = '<record><controlfield tag="000">leader</controlfield><controlfield tag="001">1</controlfield><controlfield tag="008">controlfield</controlfield><datafield ind1=" " ind2=" " tag="245"><subfield code="a">This</subfield><subfield code="b">is the</subfield><subfield code="c">title</subfield></datafield><datafield ind1=" " ind2=" " tag="520"><subfield code="a">Description</subfield></datafield><datafield ind1=" " ind2=" " tag="520"><subfield code="a">Another description</subfield><subfield code="a">Repeated subfield</subfield></datafield><datafield ind1=" " ind2=" " tag="650"><subfield code="a">Header</subfield><subfield code="0">1</subfield></datafield><datafield ind1=" " ind2=" " tag="710"><subfield code="a">Another header</subfield><subfield code="0">2</subfield></datafield></record>'
     bib = Bib.from_query({'_id': 1})
     assert main.diff_texts(bib.to_xml(), control) == []
+
+    control = control.replace('<controlfield tag="001">1</controlfield>', '')
+    assert main.diff_texts(bib.to_xml(write_id=False), control) == []
     
 def test_xml_encoding():
     from dlx.marc import Bib
@@ -755,17 +758,19 @@ def test_to_mrk(bibs):
     from dlx.marc import Bib
     
     control = '=000  leader\n=001  1\n=008  controlfield\n=245  \\\\$aThis$bis the$ctitle\n=520  \\\\$aDescription\n=520  \\\\$aAnother description$aRepeated subfield\n=650  \\\\$aHeader$01\n=710  \\\\$aAnother header$02\n'
-
     bib = Bib.from_query({'_id': 1})
     assert bib.to_mrk() == control
 
+    control = '\n'.join([line for line in control.split("\n") if line[:4] != '=001'])
+    assert bib.to_mrk(write_id=False) == control
+
 def test_from_mrk(db):
     from dlx.marc import Bib
     
     control = '=000  leader\n=001  1\n=008  controlfield\n=245  \\\\$aThis$bis the$ctitle\n=520  \\\\$aDescription\n=520  \\\\$aAnother description$aRepeated subfield\n=650  \\\\$aHeader$01\n=710  \\\\$aAnother header$02\n'
 
     bib = Bib.from_mrk(control, auth_control=True)
-    bib.id = 1
+    assert bib.id == 1
     assert bib.to_mrk() == control
     assert bib.commit(auth_check=True)
 
@@ -915,9 +920,10 @@ def test_from_xml():
     
     bib = Bib.from_xml('<record><controlfield tag="001">1</controlfield><datafield tag="245" ind1=" " ind2=" "><subfield code="a">Title</subfield><subfield code="a">Repeated</subfield></datafield></record>')        
     assert bib.get_value('001', None) == '1'
+    assert bib.id == 1
     assert bib.get_value('245', 'a') == 'Title'
     assert bib.get_value('245', 'a', address=[0, 1]) == 'Repeated'
-        
+
 def test_auth_use_count(db, bibs, auths):
     from dlx.marc import Bib, Auth
     
diff --git a/tests/test_marcset.py b/tests/test_marcset.py
index 25857e00..1b11aef9 100644
--- a/tests/test_marcset.py
+++ b/tests/test_marcset.py
@@ -93,19 +93,25 @@ def test_from_table(db):
     from dlx.util import Table
     
     table = Table([
-        ['1.246$a',  '1.246$b',  '1.269$c', '2.269$c', '1.650$a', '1.650$0'],
-        ['title', 'subtitle', '1999-12-31','repeated', '', 1],
-        ['title2','subtitle2','2000-01-01','repeated', '', 1],
+        ['1.001', '1.246$a',  '1.246$b',  '1.269$c', '2.269$c', '1.650$a', '1.650$0'],
+        ['98', 'title', 'subtitle', '1999-12-31','repeated', '', 1],
+        ['99', 'title2','subtitle2','2000-01-01','repeated', '', 1],
     ])
     
     bibset = BibSet.from_table(table)
+    assert bibset.records[0].id == 98
+    assert bibset.records[1].id == 99
 
     for bib in bibset.records:
         assert bib.get_value('246','b')[:8] == 'subtitle'
         assert bib.get_values('269','c')[1] == 'repeated'
         assert bib.get_value('650', 'a') == 'Header'
         assert not bib.get_value('650', '0')
-        
+
+    bibset = BibSet.from_table(table, delete_subfield_zero=False)
+    for bib in bibset.records:
+        assert bib.get_value('650', '0')
+                                 
     with pytest.raises(Exception):
         # dupe field check
         bibset = BibSet.from_table(Table([['245a'], ['This']]), field_check='245a')
@@ -114,7 +120,6 @@ def test_from_table(db):
         # auth control
         bibset = BibSet.from_table(Table([['650a'], ['Invalid']]), auth_control=True)
 
-
 @pytest.mark.skip(reason='xlrd is obsolete. needs review')
 def test_from_excel():
     from dlx.marc import BibSet
@@ -228,8 +233,6 @@ def test_to_csv(db):
     # comma and quote handling
     bibs = BibSet()
     bibs.records += [Bib().set('245', 'a', 'A title, with a comma').set('245', 'b', 'subtitle'), Bib().set('245', 'a', 'A "title, or name" with double quotes in the middle').set('245', 'b', 'subtitle')]
-    print(bibs.to_csv(write_id=False))
-
     assert bibs.to_csv(write_id=False) == '1.245$a,1.245$b\n"A title, with a comma",subtitle\n"A ""title, or name"" with double quotes in the middle",subtitle'
 
     # bug issue 507: fields with more 10+ instances