From 67dbe844a24fd6128d87f62210e1282edc4dd92f Mon Sep 17 00:00:00 2001 From: Heinrich Lukas Weil Date: Sat, 12 Aug 2023 00:28:56 +0400 Subject: [PATCH 1/5] add orcid helper functions and tests to person --- src/ISA/ISA/JsonTypes/Person.fs | 42 ++++++++++++++++++++++++++-- tests/ISA/ISA.Tests/Person.Tests.fs | 43 +++++++++++++++++++++++++++++ 2 files changed, 82 insertions(+), 3 deletions(-) diff --git a/src/ISA/ISA/JsonTypes/Person.fs b/src/ISA/ISA/JsonTypes/Person.fs index df021fc2..0fa4e77e 100644 --- a/src/ISA/ISA/JsonTypes/Person.fs +++ b/src/ISA/ISA/JsonTypes/Person.fs @@ -8,6 +8,7 @@ open Fable.Core type Person = { ID : URI option + ORCID : string option LastName : string option FirstName : string option MidInitials : string option @@ -20,9 +21,10 @@ type Person = Comments : Comment [] option } - static member make id lastName firstName midInitials email phone fax address affiliation roles comments : Person = + static member make id orcid lastName firstName midInitials email phone fax address affiliation roles comments : Person = { ID = id + ORCID = orcid LastName = lastName FirstName = firstName MidInitials = midInitials @@ -35,8 +37,8 @@ type Person = Comments = comments } - static member create (?Id,?LastName,?FirstName,?MidInitials,?Email,?Phone,?Fax,?Address,?Affiliation,?Roles,?Comments) : Person = - Person.make Id LastName FirstName MidInitials Email Phone Fax Address Affiliation Roles Comments + static member create (?Id,?ORCID,?LastName,?FirstName,?MidInitials,?Email,?Phone,?Fax,?Address,?Affiliation,?Roles,?Comments) : Person = + Person.make Id ORCID LastName FirstName MidInitials Email Phone Fax Address Affiliation Roles Comments static member empty = Person.create () @@ -129,11 +131,45 @@ type Person = { person with Comments = Some comments } + static member orcidKey = "ORCID" + + static member setOrcidFromComments (person : Person) = + let isOrcidComment (c : Comment) = + c.Name.IsSome && (c.Name.Value.ToUpper().EndsWith(Person.orcidKey)) + let orcid,comments = + person.Comments + |> Option.map (fun comments -> + let orcid = + comments + |> Array.tryPick (fun c -> if isOrcidComment c then c.Value else None) + let comments = + comments + |> Array.filter (isOrcidComment >> not) + |> Option.fromValueWithDefault [||] + (orcid, comments) + ) + |> Option.defaultValue (None, person.Comments) + {person with ORCID = orcid; Comments = comments} + + static member setCommentFromORCID (person : Person) = + let comments = + match person.ORCID, person.Comments with + | Some orcid, Some comments -> + let comment = Comment.create (Name = Person.orcidKey, Value = orcid) + Array.append comments [|comment|] + |> Some + | Some orcid, None -> + [|Comment.create (Name = Person.orcidKey, Value = orcid)|] + |> Some + | None, comments -> comments + {person with Comments = comments} + member this.Copy() : Person = let nextComments = this.Comments |> Option.map (Array.map (fun c -> c.Copy())) let nextRoles = this.Roles |> Option.map (Array.map (fun c -> c.Copy())) Person.make this.ID + this.ORCID this.LastName this.FirstName this.MidInitials diff --git a/tests/ISA/ISA.Tests/Person.Tests.fs b/tests/ISA/ISA.Tests/Person.Tests.fs index 3efac474..4275cbaf 100644 --- a/tests/ISA/ISA.Tests/Person.Tests.fs +++ b/tests/ISA/ISA.Tests/Person.Tests.fs @@ -10,6 +10,48 @@ open Expecto let private createTestPerson() = Person.create(FirstName="Kevin", LastName="Frey",Email="MyAwesomeMail@Awesome.mail") +let private tests_ORCID = + testList "ORCID" [ + testCase "fromComment correctORCID" (fun () -> + let comment = Comment.create(Name = Person.orcidKey, Value = "0000-0002-1825-0097") + let p = Person.create(FirstName="My", LastName="Dude",Comments =[|comment|]) + let newP = Person.setOrcidFromComments p + Expect.isSome newP.ORCID "ORCID should now be set" + Expect.equal newP.ORCID.Value "0000-0002-1825-0097" "ORCID not taken correctly" + Expect.isNone newP.Comments "Comments not set to None" + ) + testCase "fromComment wrongORCID" (fun () -> + let comment = Comment.create(Name = "WrongKey", Value = "0000-0002-1825-0097") + let p = Person.create(FirstName="My", LastName="Dude",Comments =[|comment|]) + let newP = Person.setOrcidFromComments p + Expect.isNone newP.ORCID "ORCID should not have been taken" + Expect.isSome newP.Comments "Comments should still be there" + Expect.equal newP.Comments.Value.Length 1 "Comments should still be there" + ) + testCase "fromComment deprecatedORCID" (fun () -> + let comment = Comment.create(Name = "Investigation Person Orcid", Value = "0000-0002-1825-0097") + let p = Person.create(FirstName="My", LastName="Dude",Comments =[|comment|]) + let newP = Person.setOrcidFromComments p + Expect.isSome newP.ORCID "ORCID should now be set" + Expect.equal newP.ORCID.Value "0000-0002-1825-0097" "ORCID not taken correctly" + Expect.isNone newP.Comments "Comments not set to None" + ) + testCase "toComment SomeORCID" (fun () -> + let p = Person.create(FirstName="My", LastName="Dude",ORCID = "0000-0002-1825-0097") + let newP = Person.setCommentFromORCID p + Expect.isSome newP.Comments "Comments should now be set" + Expect.equal newP.Comments.Value.Length 1 "Comments should now be set" + Expect.equal newP.Comments.Value.[0].Name.Value Person.orcidKey "Comments should now be set" + Expect.equal newP.Comments.Value.[0].Value.Value "0000-0002-1825-0097" "Comments should now be set" + ) + testCase "toComment NoneORCID" (fun () -> + let p = Person.create(FirstName="My", LastName="Dude") + let newP = Person.setCommentFromORCID p + Expect.isNone newP.Comments "Comments should not be set" + ) + + ] + let private tests_Copy = testList "Copy" [ testCase "test copy" <| fun _ -> // this test is silly in dotnet as record types are immutable. Test this in js native too! @@ -24,4 +66,5 @@ let private tests_Copy = testList "Copy" [ let main = testList "Person" [ tests_Copy + tests_ORCID ] \ No newline at end of file From a1644839a5a1518063ff9dc9488c4ed7ebc85fb3 Mon Sep 17 00:00:00 2001 From: Heinrich Lukas Weil Date: Sat, 12 Aug 2023 00:39:47 +0400 Subject: [PATCH 2/5] add orcid logic and tests to json parsing --- src/ISA/ISA.Json/Person.fs | 5 ++- tests/ISA/ISA.Json.Tests/Json.Tests.fs | 44 +++++++++++++++++++ .../ISA/ISA.Json.Tests/TestObjects/Person.fs | 14 ++++++ 3 files changed, 62 insertions(+), 1 deletion(-) diff --git a/src/ISA/ISA.Json/Person.fs b/src/ISA/ISA.Json/Person.fs index fb32bf96..8c479898 100644 --- a/src/ISA/ISA.Json/Person.fs +++ b/src/ISA/ISA.Json/Person.fs @@ -32,8 +32,9 @@ module Person = | _ -> "#EmptyPerson" let rec encoder (options : ConverterOptions) (oa : obj) = + let oa = oa :?> Person |> Person.setCommentFromORCID [ - if options.SetID then "@id", GEncode.string (oa :?> Person |> genID) + if options.SetID then "@id", GEncode.string (oa |> genID) else GEncode.tryInclude "@id" GEncode.string (oa |> GEncode.tryGetPropertyValue "ID") if options.IncludeType then "@type", GEncode.string "Person" GEncode.tryInclude "firstName" GEncode.string (oa |> GEncode.tryGetPropertyValue "FirstName") @@ -54,6 +55,7 @@ module Person = Decode.object (fun get -> { ID = get.Optional.Field "@id" GDecode.uri + ORCID = None FirstName = get.Optional.Field "firstName" Decode.string LastName = get.Optional.Field "lastName" Decode.string MidInitials = get.Optional.Field "midInitials" Decode.string @@ -65,6 +67,7 @@ module Person = Roles = get.Optional.Field "roles" (Decode.array (OntologyAnnotation.decoder options)) Comments = get.Optional.Field "comments" (Decode.array (Comment.decoder options)) } + |> Person.setOrcidFromComments ) diff --git a/tests/ISA/ISA.Json.Tests/Json.Tests.fs b/tests/ISA/ISA.Json.Tests/Json.Tests.fs index 835093d4..086405f4 100644 --- a/tests/ISA/ISA.Json.Tests/Json.Tests.fs +++ b/tests/ISA/ISA.Json.Tests/Json.Tests.fs @@ -881,6 +881,48 @@ let testPersonFile = mySequenceEqual actual expected "Written person file does not match read person file" ) + + + testCase "WithORCID ReaderCorrectness" (fun () -> + + let p = Person.fromString TestObjects.Person.personWithORCID + Expect.isNone p.Comments "Comments should be None" + Expect.isSome p.ORCID "ORCID should be Some" + Expect.equal p.ORCID.Value "0000-0002-1825-0097" "ORCID not as expected" + + ) + + testAsync "WithORCID WriterSchemaCorrectness" { + + let a = Person.fromString TestObjects.Person.personWithORCID + + let s = Person.toString a + + let! validation = Validation.validatePerson s + + Expect.isTrue validation.Success $"Person did not match schema: {validation.GetErrors()}" + } + + testCase "WithORCID OutputMatchesInput" (fun () -> + + let o = + Person.fromString TestObjects.Person.personWithORCID + |> Person.toString + + let expected = + TestObjects.Person.personWithORCID + |> Utils.extractWords + |> Array.countBy id + |> Array.sortBy fst + + let actual = + o + |> Utils.extractWords + |> Array.countBy id + |> Array.sortBy fst + + mySequenceEqual actual expected "Written person file does not match read person file" + ) ] let testPersonFileLD = @@ -1265,6 +1307,7 @@ let testInvestigationFile = let person = Person.make (Some "Persons/LukasWeil") + None (Some "Weil") (Some "Lukas") (Some "H") @@ -1687,6 +1730,7 @@ let testInvestigationFileLD = let person = Person.make (Some "Persons/LukasWeil") + None (Some "Weil") (Some "Lukas") (Some "H") diff --git a/tests/ISA/ISA.Json.Tests/TestObjects/Person.fs b/tests/ISA/ISA.Json.Tests/TestObjects/Person.fs index c7430b33..53cacd1e 100644 --- a/tests/ISA/ISA.Json.Tests/TestObjects/Person.fs +++ b/tests/ISA/ISA.Json.Tests/TestObjects/Person.fs @@ -26,6 +26,20 @@ let person = } """ +let personWithORCID = + """ + { + "firstName": "Juan", + "lastName": "Castrillo", + "comments": [ + { + "value": "0000-0002-1825-0097", + "name": "ORCID" + } + ] + } + """ + let personLD = """ { From 5c630d0e3e90e5decd3c89820c0a4ebf893f34a6 Mon Sep 17 00:00:00 2001 From: Heinrich Lukas Weil Date: Sat, 12 Aug 2023 01:06:47 +0400 Subject: [PATCH 3/5] add orcid handling to isa spreadsheet parser and updated test --- .../ISA.Spreadsheet/InvestigationFile/Contacts.fs | 3 +++ tests/ISA/ISA.Spreadsheet.Tests/AssayFileTests.fs | 12 ++++++++++++ tests/ISA/ISA.Spreadsheet.Tests/TestObjects/Assay.fs | 8 ++++++++ 3 files changed, 23 insertions(+) diff --git a/src/ISA/ISA.Spreadsheet/InvestigationFile/Contacts.fs b/src/ISA/ISA.Spreadsheet/InvestigationFile/Contacts.fs index 8e0b257f..4385afbb 100644 --- a/src/ISA/ISA.Spreadsheet/InvestigationFile/Contacts.fs +++ b/src/ISA/ISA.Spreadsheet/InvestigationFile/Contacts.fs @@ -25,6 +25,7 @@ module Contacts = let roles = OntologyAnnotation.fromAggregatedStrings ';' role rolesTermSourceREF rolesTermAccessionNumber Person.make None + None (Option.fromValueWithDefault "" lastName ) (Option.fromValueWithDefault "" firstName ) (Option.fromValueWithDefault "" midInitials) @@ -35,6 +36,7 @@ module Contacts = (Option.fromValueWithDefault "" affiliation) (Option.fromValueWithDefault [||] roles ) (Option.fromValueWithDefault [||] comments ) + |> Person.setOrcidFromComments let fromSparseTable (matrix : SparseTable) = if matrix.ColumnCount = 0 && matrix.CommentKeys.Length <> 0 then @@ -67,6 +69,7 @@ module Contacts = let matrix = SparseTable.Create (keys = labels,length=persons.Length + 1) let mutable commentKeys = [] persons + |> List.map Person.setCommentFromORCID |> List.iteri (fun i p -> let i = i + 1 let rAgg = Option.defaultValue [||] p.Roles |> OntologyAnnotation.toAggregatedStrings ';' diff --git a/tests/ISA/ISA.Spreadsheet.Tests/AssayFileTests.fs b/tests/ISA/ISA.Spreadsheet.Tests/AssayFileTests.fs index bc9c8393..07addcdf 100644 --- a/tests/ISA/ISA.Spreadsheet.Tests/AssayFileTests.fs +++ b/tests/ISA/ISA.Spreadsheet.Tests/AssayFileTests.fs @@ -29,6 +29,18 @@ let testMetaDataFunctions = ) + testCase "ReaderReadsORCID" (fun () -> + + let assay = ArcAssay.fromMetadataSheet TestObjects.Assay.assayMetadata + Expect.equal assay.Performers.Length 3 "Assay should have 3 performers" + Expect.isSome assay.Performers.[0].ORCID "ORCID should be set" + Expect.equal assay.Performers.[0].ORCID.Value "0000-0002-1825-0097" "ORCID not read correctly" + Expect.isNone assay.Performers.[1].ORCID "ORCID should not be set" + Expect.isSome assay.Performers.[2].ORCID "ORCID should be set" + Expect.equal assay.Performers.[2].ORCID.Value "0000-0002-1825-0098" "ORCID not read correctly" + + ) + testCase "ReaderSuccessObsoleteSheetName" (fun () -> let readingSuccess = diff --git a/tests/ISA/ISA.Spreadsheet.Tests/TestObjects/Assay.fs b/tests/ISA/ISA.Spreadsheet.Tests/TestObjects/Assay.fs index fa32fa0d..ca90cb9d 100644 --- a/tests/ISA/ISA.Spreadsheet.Tests/TestObjects/Assay.fs +++ b/tests/ISA/ISA.Spreadsheet.Tests/TestObjects/Assay.fs @@ -76,6 +76,10 @@ let assayMetadata = row22.[2].Value <- "Sheet1;Sheet2" row22.[3].Value <- "Sheet2" row22.[4].Value <- "Sheet3" + let row23 = ws.Row(23) + row23.[1].Value <- "Comment[ORCID]" + row23.[2].Value <- "0000-0002-1825-0097" + row23.[4].Value <- "0000-0002-1825-0098" ws let assayMetadataDeprecatedKeys = @@ -152,6 +156,10 @@ let assayMetadataDeprecatedKeys = row22.[2].Value <- "Sheet1;Sheet2" row22.[3].Value <- "Sheet2" row22.[4].Value <- "Sheet3" + let row23 = ws.Row(23) + row23.[1].Value <- "Comment[Assay ORCID]" + row23.[2].Value <- "0000-0002-1825-0097" + row23.[4].Value <- "0000-0002-1825-0098" ws let assayMetadataObsoleteSheetName = From 296de795c546ad3444269281843c910f8951a421 Mon Sep 17 00:00:00 2001 From: Heinrich Lukas Weil Date: Sat, 12 Aug 2023 01:15:35 +0400 Subject: [PATCH 4/5] stop isa metadata spreadsheet writer from writing empty strings --- src/ISA/ISA.Spreadsheet/SparseTable.fs | 3 ++- tests/ISA/ISA.Spreadsheet.Tests/AssayFileTests.fs | 10 ++++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/src/ISA/ISA.Spreadsheet/SparseTable.fs b/src/ISA/ISA.Spreadsheet/SparseTable.fs index 338793b3..7c72e438 100644 --- a/src/ISA/ISA.Spreadsheet/SparseTable.fs +++ b/src/ISA/ISA.Spreadsheet/SparseTable.fs @@ -48,7 +48,8 @@ module SparseRow = let writeToSheet (rowI : int) (row : SparseRow) (sheet : FsWorksheet) = let fsRow = sheet.Row(rowI) row - |> Seq.iter (fun (colI,v) -> fsRow.[colI + 1].SetValueAs v) + |> Seq.iter (fun (colI,v) -> + if v.Trim() <> "" then fsRow.[colI + 1].SetValueAs v) type SparseTable = diff --git a/tests/ISA/ISA.Spreadsheet.Tests/AssayFileTests.fs b/tests/ISA/ISA.Spreadsheet.Tests/AssayFileTests.fs index 07addcdf..44000340 100644 --- a/tests/ISA/ISA.Spreadsheet.Tests/AssayFileTests.fs +++ b/tests/ISA/ISA.Spreadsheet.Tests/AssayFileTests.fs @@ -68,6 +68,16 @@ let testMetaDataFunctions = Expect.isOk writingSuccess (Result.getMessage writingSuccess) ) + testCase "WriterCreatesNoEmptyCells" (fun () -> + + let o = + ArcAssay.fromMetadataSheet TestObjects.Assay.assayMetadata + |> ArcAssay.toMetadataSheet + + o.CellCollection.GetCells() + |> Seq.iter (fun c -> Expect.notEqual (c.Value.Trim()) "" $"Cell {c.Address.ToString()} should not contain empty string") + ) + testCase "WriterSuccessObsoleteSheetName" (fun () -> let a = ArcAssay.fromMetadataSheet TestObjects.Assay.assayMetadataObsoleteSheetName From 732eb625222f9a2e476b3981dd12854a75569c07 Mon Sep 17 00:00:00 2001 From: Heinrich Lukas Weil Date: Sat, 12 Aug 2023 01:35:46 +0400 Subject: [PATCH 5/5] fix native js test failing because of orcid --- ARCtrl.sln | 1 + tests/JavaScript/ISA.Person.js | 6 ++++-- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/ARCtrl.sln b/ARCtrl.sln index a2006831..8ceaef60 100644 --- a/ARCtrl.sln +++ b/ARCtrl.sln @@ -55,6 +55,7 @@ Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "playground", "playground", EndProject Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "JavaScript", "JavaScript", "{913222CA-261F-49CB-A823-CC7C335F964A}" ProjectSection(SolutionItems) = preProject + tests\JavaScript\ISA.Person.js = tests\JavaScript\ISA.Person.js tests\JavaScript\Main.js = tests\JavaScript\Main.js EndProjectSection EndProject diff --git a/tests/JavaScript/ISA.Person.js b/tests/JavaScript/ISA.Person.js index 25fadc2f..339cd541 100644 --- a/tests/JavaScript/ISA.Person.js +++ b/tests/JavaScript/ISA.Person.js @@ -5,8 +5,10 @@ import { Comment$ as Comment } from "./ARCtrl/ISA/ISA/JsonTypes/Comment.js"; describe('ISA.Person', function () { describe('copy', function () { it('check mutable', function () { - const person = Person.create(void 0, "Frey", "Kevin", void 0, "MyAwesomeMail@Awesome.mail"); + const person = Person.create(void 0, "0000-0002-1825-0097", "Frey", "Kevin", void 0, "MyAwesomeMail@Awesome.mail"); const copy = person.Copy() + equal(person.ORCID, "0000-0002-1825-0097") + equal(copy.ORCID, "0000-0002-1825-0097") equal(person.FirstName, "Kevin"); equal(copy.FirstName, "Kevin"); person.FirstName = "NotKevin" @@ -15,7 +17,7 @@ describe('ISA.Person', function () { }); it('check nested mutable', function(){ const comment = Comment.fromString("TestKey", "TestValue") - const person = Person.create(void 0, "Frey", "Kevin", void 0, "MyAwesomeMail@Awesome.mail", void 0, void 0, void 0, void 0, void 0, [comment]); + const person = Person.create(void 0, void 0, "Frey", "Kevin", void 0, "MyAwesomeMail@Awesome.mail", void 0, void 0, void 0, void 0, void 0, [comment]); const copy = person.Copy() equal(person.Comments.length,1) deepEqual(person.Comments,copy.Comments, "Should be same")