Project

General

Profile

Enhancement #2685

Opensearch abstraktionslag (SAL)

Added by Rolf Madsen almost 2 years ago. Updated over 1 year ago.

Status:
Resolved (tag version)
Priority:
Normal
Estimated time:
URL med eksempel:
Kategorier:
Driftsvedligehold - Refaktorering (Opdatering af kodebasen), Integration - Brønd - Søg, rankér filtrér sortér, Integration - Brønd - Data og relationer

Description

Indledning og historik

DDB har modtaget en henvendelse fra The Reykjavík City Library på Island, som ønsker at benytte DDB CMS som grundlag for de Islandske bibliotekers digitale services og hjemmesider, som erstatning til deres eksisterende separate OPAC’s og hjemmesider, som vi kender dem fra før Ding og DDB CMS’ udbredelse.

For hurtigst muligt at nå denne målsætning ønsker Island at bygge videre på det arbejde, vi allerede har opnået med DDB CMS.

Udviklingsopgaven udføres af Reload A/S, som også har udført en grundig teknisk foranalyse i januar 2017.

Udfordringen er at Island benytter Primo som databrønd, og DDB CMS er kun bygget til at understøtte søgninger og opslag i Databrønden.
Løsningsforslag

DDB CMS er bygget på en måde, så man, via et abstraktionslag, kan integrere til forskellige bibliotekssystemer, og derfor kan man forholdsvis let lave en udvidelse til Aleph, som er det bibliotekssystem, der benyttes på Island.

For at kunne integrere til Primo, i stedet for den databrønd vi har i Danmark, er det nødvendigt at bygge et abstraktionslag efter samme model, som vi i DDB CMS allerede benytter til bibliotekssystemer.

Island vil finansiere og bygge:

  1. Databrønd abstraktionslaget og automatiske tests
  2. Integrationen til Aleph
  3. Integrationen til Primo

Integrationerne til Aleph og Primo udvikler, vedligeholder og implementerer de islandske biblioteker selv.

DDB’s forudsætninger for at godkende integrationen er at:

  1. Island finansierer et godkendt code review
  2. Island finansierer et eventuelt gap der måtte opstå på grund af den sideløbende udvikling af DDB CMS inden løsningen implementeres i DDB CMS
  3. DDB til enhver tid har ret til at afvise Islands bidrag til DDB CMS kodebasen.

Vores oplæg er at integrationen udrulles i den første release EFTER DDB CMS 2017 2. opgradering.
Argumenter for integration af databrønd abstraktionslaget:

Overvejelser omkring udvikling og integration af Opensearch abstraktionslaget

  1. DDB CMS integrationen til Databrønden er løbende omskrevet til flere opensearch versioner mv., og den afledte kompleksitet vil blive reduceret med implementeringen af abstraktionslaget.
  2. De automatiske tests, der udvikles til abstraktionslaget, vil reducere det fremtidige behov for DDB personaleressourcer til at udføre manuelle brugertests og bidrage til højere kvalitet og hurtigere udrulning.
  3. DDB CMS vil kunne drage nytte af fremtidige udviklingsarbejder fra Island
  4. Med databrønd abstraktionslaget kunne DDB CMS blive interessant for andre biblioteker, herunder FFU bibliotekerne.
  5. Databrønd abstraktionslaget er en meget central komponent, men den én til én omskrivning der er tale om fra den eksisterende integrationsflade til databrønden og de automatiske tests gør ifølge Reload at risikoen ved skiftet burde være minimal.
  6. På sigt vil en omskrivning af søgegrænsefladen, f. eks. til at understøtte openplatform, gøre databrønd abstraktionslaget i DDB CMS irrelevant.
  7. Implementering af databrønd abstraktionslaget vil på trods af indførelsen af automatiske tests betyde at der også skal udføres manuelle tests af alle berørte funktioner hvilket DDB CMS teamet estimeret til tre dages arbejde.

History

#1 Updated by Rolf Madsen almost 2 years ago

  • Status changed from Ready for development to Development

#2 Updated by Gitte Barlach over 1 year ago

  • Description updated (diff)

Hej Kasper

Kan I fremsætte et PR, så Jesper kan lave et indledende review?

#3 Updated by Kasper Garnæs over 1 year ago

  • Assignee changed from Kasper Garnæs to Jesper Kristensen

Vi har oprettet et PR til det initielle code review her: https://github.com/ding2/ding2/pull/894.

#4 Updated by Jesper Kristensen over 1 year ago

Da dette er et pre-review har jeg valgt ikke at skrive på github og komme ind på de enkelte kode linjer, da der stadig arbejdes på koden.

Overordnet ser koden rigtig god og veldokumenteret ud og overholder kodestandart for DDB CMS projekter. Det virker til at være en gennemtænkt til gang til at lave et søge abstraktionslag (SAL).

Jeg har nogle enkelt punkt som projekt og DDB skal være opmærksom på:

  • Har det været oppe og vend om man skulle indføre en cache i dette lag og derved kunne simplificer de enkelte søge provider. Mod argumentet kan være at der ikke er tilstrækkelig information til at lave denne optimering i SAL.
  • Jeg er ikke helt sikker på hvilke begrænsninger dette lag indføre I forhold til andre moduler i CMS’et. Er det noget projekter har haft oppe og vend (tænker her f.eks. i forhold til TingSearchCommonFields og om det vil begrænse hvad man kan lave af søgninger etc.?).
  • Har projekt testet implantation mod open-search på en kopi af et ”live” DDB CMS bibliotek for at sikker at alt virker efterhensigten?  
  • Der sker migration af rettigheder i open-search (det skulle secure permission kunne håndtere).
  • Dette ændre flere sted ved hvordan man konfigurer integration med DBC services og derved skal vi huske at opdaterer dokumentation til super-bruger af CMS’et.
  • Et heads-up til projekt omkring ADHL er på vej ud af koden, da der ikke meget længer vil komme ny data til services grundet manglede support fra FSB.

Jeg kunne godt tænke mig at der måske blev laver nogle ”små” performance tests af CMS’et med og uden SAL for at se om dette ekstra lag har nogle indflydelse på performance. Specielt hvis man tænker noget cache ind I SAL.

Der er endnu en lang række ”todo’s” rundt omkring i koden med spørgsmål og tilpasninger. Men dette er forventeligt ved et pre-review.

@DDB skal være opmærksom på at SAL vil ændre ved store set alle aspekter af kommunikation med DBC brønd services. Der er ændringer i moduler på tværs af næsten hele systemet og derfor vil SAL medføre en påkrævet testning af hele CMS’et.

#5 Updated by Rolf Madsen over 1 year ago

  • Assignee changed from Jesper Kristensen to Kasper Garnæs

Jeg har en enkelt kommentar i forhold til punktet:

Et heads-up til projekt omkring ADHL er på vej ud af koden, da der ikke meget længer vil komme ny data til services grundet manglede support fra FSB.

FBS sender pt. ikke udlånsdata til ADHL servicen, men det forventer vi bliver etableret i 2018.

Indtil videre er det derfor kun de to resterende biblioteker på DDELibra, nemlig Sønderborg og Hjørring, der sender data til ADHL.

#6 Updated by Gitte Barlach over 1 year ago

  • Subject changed from Opensearch abstraktionslag to Opensearch abstraktionslag (SAL)

#7 Updated by Rolf Madsen over 1 year ago

  • Target version changed from Release 29-2 - Bugfixes (7.x-4.5.0) to Release 28 - Søge abstraktionslag (SAL) - (7.x-4.3.1)

#8 Updated by Kasper Garnæs over 1 year ago

  • Assignee changed from Kasper Garnæs to Jesper Kristensen

Koden er blevet videreudviklet og opdateret i forhold til kommentarerne fra det initielle review. Det gamle PR er derfor ikke længere relevant og var fyldt med kommentarer fra GitCop.

Jeg har derfor oprettet et nyt: https://github.com/ding2/ding2/pull/982.

#9 Updated by Kasper Garnæs over 1 year ago

  • Status changed from Development to Needs code review

#10 Updated by Mads Høgstedt Danquah over 1 year ago

Ang spørgsmålene fra #4

Har det været oppe og vend om man skulle indføre en cache i dette lag og derved kunne simplificer de enkelte søge provider. Mod argumentet kan være at der ikke er tilstrækkelig information til at lave denne optimering i SAL.

Vi har overvejet det, hovedårsagen til at vi lod være her i første omgang er at man hurtigt bliver nødt til at stille krav til hvordan søge-provideren implementerer f.eks. TingObjectInterface (objektet der erstatter ->reply propertien på TingEntity). Hvis dette skal kunne caches må det enten ikke indeholde ikke-serialiser-bare objekter (som f.eks. et DOMDocument), eller også skal der være en eksplicit måde at kunne forberede objektet til serialisering. Alternativt skal man droppe interfacet og simpelthen kræve at provideren flytter al data over i (i dette tilfælde) en instans af TingObject value-objektet som vi ved kan serialiseres. 

Men, på den korte bane har vi valgt at lade opensearch og primo-implementationen af søge-providere have deres egen interne caching. 

Jeg er ikke helt sikker på hvilke begrænsninger dette lag indføre I forhold til andre moduler i CMS’et. Er det noget projekter har haft oppe og vend (tænker her f.eks. i forhold til TingSearchCommonFields og om det vil begrænse hvad man kan lave af søgninger etc.?).

Der vil uden tvivl være begræsninger, to store områder

  1. Felter, hvis man skal holde sig kompatibel må man holde sig til filter i TingSearchCommonFields
  2. Filtrering, TingSearchFieldFilter er lige nu begrænset til at lave en equality filtrering, så man kan ikke udtrykke f.eks. "<feltnavn> > værdi"

Generelt vil man i de her tilfælde skulle vælge imellem

  • At udvide SAL - her kommer man til at skulle holde et øje på hvad de andre providere rent faktisk har mulighed for at understøtte og overveje om man kan lave sin funktionalitet valgfri i tilfælde af at de ikke gør
  • At bryde kompabiliteten - f.eks. ved at bruge et feltnavn der ikke er en del af TingSearchCommonFields - dette vil umiddelbart kun være ok i situationer hvor man ved hvilken provider der er i brug, f.eks. ville det være ok fra opesearch.module
  • At omgå SAL - enten inde fra selve søge-provideren der f.eks. kan vælge at skifte opførsel på baggrund af en variabel, eller ved at f.eks. i Open Search tilfælde at bruge tingclient direkte - det bør igen kun give mening at gøre i situationer hvor man er 100% sikker på at koden kun ville blive afviklet i installationer der bruger opensearch.

Det ser nok selvsagt at dette er et risikabelt område at færdes i.

Har projekt testet implantation mod open-search på en kopi af et ”live” DDB CMS bibliotek for at sikker at alt virker efterhensigten?  

Nej, vi har primært testet imod connie som "bibliotekssystem" og https://opensearch.addi.dk/b3.5_4.5/ med test-profilen som søge-system

 

Jeg så de resterende kommentarer som konstateringer - men sig endelig til hvis der også var noget der skulle besvares dér eller hvis der er andre spørgsmål.

#11 Updated by Mads Høgstedt Danquah over 1 year ago

Jeg missede lige

Der sker migration af rettigheder i open-search (det skulle secure permission kunne håndtere).

Vi indfører en ny "administer opensearch settings" permission men lade de foregående "administer ting settings" "core" permission blive. Fremadrettet er tanken at "administer ting settings" er core settings der har med søge-providere generelt at gøre, og eg. "administer opensearch settings" handler om at må tilgå opensearch søge providerens interne konfigurationer.

Migreringen kan umiddelbart ikke klares via secure permission (mere specifikt ding_permissions.module) da den kun dækker core-permissions, og derfor ikke kan referere til "administer opensearch settings" da den permission eksempelvis ikke vil eksistere i den islandske installation.

#12 Updated by Jesper Kristensen over 1 year ago

ding_permission skal dække alle permission i DDB CMS. 

Hvis en permisison ikke er der i skal de bare tilføjes. Så jeg forstår ikke helt det du skriver!

#13 Updated by Mads Høgstedt Danquah over 1 year ago

Kan også være vi var lidt for forsigtige.

Antagelsen var at ding_permissions kunne komme i problemer hvis den refererer til en permission der ikke findes. I den Islandske installation er opensearch.module ikke enablet, dvs "administer opensearch settings" findes ikke.

Men det kan da meget vel være at secure_permission faktisk håndterer scenariet og så bare ikke forsøger at give permissions der ikke findes?

#14 Updated by Jesper Kristensen over 1 year ago

Det er nu udført kode review på det nyeste PR, og der er 175 kommentarer. Det kan virke voldsomt, men SAL er en meget central del af kodeen i DDB CMS og det rører ved store set alle dele af kodebasen. Så derfor er der en del "nitpicking" kommentarer til koden. Der er en del opklarende sprøgsmål, forslag mm. Derudover er der en del mindre kode style ting (på en eller anden måde er det PR. kommet uden om Scrutinizer).

Der er rigtig mange @todo's tilbage i koden, som vi enten skal have rettet før SAL kommer i core eller vi skal have oprettet et legacy mini-projekt her på Redmine og lavet tickets på dem alle samme (@gitte og @rolf dette må være op til jer og tidsplan mm.).

Jeg har patchet koden på en installation af DDB CMS og fik følgende notices:

http://ddb-dev.vm/admin/config/ting/search

Notice: Undefined offset: 0 i opensearch_search_sort_options() (linje 559 af vagrant/htdocs/profiles/ding2/modules/opensearch/includes/opensearch.search.inc).

http://ddb-dev.vm/search/ting/potter:

* Warning: Invalid argument supplied for foreach() i OpenSearch\OpenSearchTingObject->getOnlineUrl() (linje 225 af /vagrant/htdocs/profiles/ding2/modules/opensearch/src/OpenSearchTingObject.php).
* Warning
: Invalid argument supplied for foreach() i OpenSearch\OpenSearchTingObject->getOnlineUrl() (linje 225 af /vagrant/htdocs/profiles/ding2/modules/opensearch/src/OpenSearchTingObject.php).
* Warning
: Invalid argument supplied for foreach() i OpenSearch\OpenSearchTingObject->getOnlineUrl() (linje 225 af /vagrant/htdocs/profiles/ding2/modules/opensearch/src/OpenSearchTingObject.php).
* Warning
: strtr(): The second argument is not an array i ting_field_formatter_view() (linje 479 af /vagrant/htdocs/profiles/ding2/modules/ting/ting.field.inc).
* Warning
: Invalid argument supplied for foreach() i OpenSearch\OpenSearchTingObject->getOnlineUrl() (linje 225 af /vagrant/htdocs/profiles/ding2/modules/opensearch/src/OpenSearchTingObject.php).
* Notice
: Undefined offset: 0 i ting_field_formatter_view() (linje 478 af /vagrant/htdocs/profiles/ding2/modules/ting/ting.field.inc).
* Warning
: strtr(): The second argument is not an array i ting_field_formatter_view() (linje 479 af /vagrant/htdocs/profiles/ding2/modules/ting/ting.field.inc).
* Warning
: Invalid argument supplied for foreach() i OpenSearch\OpenSearchTingObject->getOnlineUrl() (linje 225 af /vagrant/htdocs/profiles/ding2/modules/opensearch/src/OpenSearchTingObject.php).

I forhold til migration af konfiguration fra ting til opensearch:

 * @Gitte @rolf så er dette for sites hostet ved DBC sat via settings.php. Så hvis DBC vil blive ved med at "overskrive/enforce" Brøndindstillingerne mm. skal DBC i store træk rename ting_* settings til opensearch_* i alle settings.php filere for alle sites.

Klient koden til opensearch hedder stadigvæk ting-client. Skulle den på sigt omdåbes til opensearch-client for at fjerne misforståelser i kode basen? Tror det er uden for SAL projektets opgave, men kunne være vi skulle have en ticket på at gøre dette med mindre SAL vil tilbyde dette?

Under modules/ting/src/Search/* mangler der en hel del "type hints" siger PHP code sniffer, jeg har ikke lavet kommentar til alle disse. Men generelt vil det være en god idé at køre:

phpcs --standard=Drupal modules/opensearch/ --ignore=lib

Så skal DDB huske at der osse er ændringer til hvor man konfigure forskellige ting etc. så der er osse en opgave med at opdatere wiki og dokumentation for slut-bruger.

#15 Updated by Jesper Kristensen over 1 year ago

  • Status changed from Needs code review to Reviewed - Needs info/rework
  • Assignee changed from Jesper Kristensen to Kasper Garnæs

#16 Updated by Kasper Garnæs over 1 year ago

  • Tracker changed from Bug to Enhancement
  • Assignee changed from Kasper Garnæs to Mads Høgstedt Danquah

Jeg har gennemgået alle kommentarer. Mange ting er blevet retter - også selv om en del stammer fra endnu ældre kode.

 

Der er et par moduler som vi ikke har nået at migrere endnu. Derfor foreslår vi at der oprettes tickets for dem så vi kan arbejde videre med dem efterfølgende. Deres nuværende tilstand vil ikke skade nuværende sites. Det gælder følgende moduler:

 

  • BPI
  • Ting
  • Ding Message
  • Ding Serendipity Entity

 

Vi vil derfor førende følgende: 

 

  • Sørge for at PR’et stadig kan merges
  • Teste of ding_serendipity_lists og extended ting_search for at se om disse kræver nye tickets.
  • Migrere 
  • Migrere ting_new_materials for at løssen række todos

 

Bemærk at vi i PRet har en række spørgsmål til reviewer. De skal adresseres før for at se om der kræves yderligere ændringer. De drejer sig om:

 

  • Caching
  • ADHL
  • Acquisition Date
  • Code style

#17 Updated by Jesper Kristensen over 1 year ago

Synes det lyder som en plan Kasper.

#18 Updated by Jesper Kristensen over 1 year ago

Der er blevet rette en masse ting (specialet fra gamle kode der er blevet flyttet rundt).

Så det ser meget godt ud :-).

Der skal lige styre på hvem der oprette tickets på de todo's der bliver flytte til senere. Så er der nogle ting som @danquah skal svare på.

#19 Updated by Mads Høgstedt Danquah over 1 year ago

Roger - jeg løber det igennem i morgen og får svaret på det der skal svares på. Er jeg nævnt eksplicit dér hvor jeg skal svare?

#20 Updated by Kasper Garnæs over 1 year ago

  • Assignee changed from Mads Høgstedt Danquah to Kasper Garnæs

Vi har opdateret PR'et med ændringer til følgende ting:

  • Sørget for at PR’et stadig kan merges
  • Migreret ting_new_materials, bpi, ding_ekurser og ting_search for at løssen række todos

Som jeg ser det mangler følgende:

  • Jeg gennemgår de allerede opdateretede ændringer for at sikre at der er enighed med Core team
  • Core team gennemgå de sidste opdateringer.

#21 Updated by Kasper Garnæs over 1 year ago

  • Status changed from Reviewed - Needs info/rework to Needs code review
  • Assignee changed from Kasper Garnæs to Jesper Kristensen

Vi har opdateret pull requestet med rettelser til de sidste kommentarer.

Der er blevet oprettet issues for 4 emner som vil blive håndteret efterfølgende. Ingen af punkterne vil have konsekvenser for nuværende sites der benytter OpenSearch:

- Ting Agency
- Ding Message
- Ding Serendipity Entity
- Håndtering af ADHL

#22 Updated by Jesper Kristensen over 1 year ago

  • Status changed from Needs code review to Technical test
  • Assignee changed from Jesper Kristensen to Gitte Barlach

Reviewed, approved and merged

#23 Updated by Michael W. Christoffersen over 1 year ago

  • Assignee changed from Gitte Barlach to Michael W. Christoffersen

#24 Updated by Gitte Barlach over 1 year ago

  • Status changed from Technical test to Resolved (tag version)

Testet ad flere omgange på upgrade-fbs og vanilla-fbs

Vi har udført test af alt vedr. søgning, filtrering/facetter, sortering, rankering, visning med/uden stort søgefelt, med/uden søgeprofiler for anonyme og indloggede brugere. Vi har dertil testet forsidekarruselen (visning/bladring/opret/flyt/slet) samt nyheder, arrangementer og sider med tilknyttede materialer. Endvidere er reserver-flow testet samt alle lånerstatussiderne. 
Backend konfigurationsmuligheder ift. søgning er ligeledes testet. 


Godkender hermed denne. 

Also available in: Atom PDF