Project

General

Profile

Enhancement #1999

Understøttelse af tags og forbedring af billedhåndtering i BPI

Added by Carsten Kaa over 3 years ago. Updated about 2 years ago.

Status:
Resolved (tag version)
Priority:
High
Estimated time:
URL med eksempel:
Kategorier:
Integration - BPI

Description

Issue til genimplementering af 2 issues oprindeligt tiltænkt 2016-1, men som måtte trækkes ud:
#1476
#1497

Det viste sig at være manglende https understøttelse på vores staging server.
Det er samme kode som allerede er godkendt af core – eneste grund til at det ikke virkede var fordi BPI tjenesten vi testede fra lå på en staging server uden https understøttelse.

billeder_BPI.PNG (65.8 KB) billeder_BPI.PNG Carsten Vilhelmsen, 07/06/2017 11:29 AM
bpi_billeder_dump1.jpg (28 KB) bpi_billeder_dump1.jpg Carsten Vilhelmsen, 09/07/2017 01:30 PM
bpi_billeder_dump2.jpg (66.8 KB) bpi_billeder_dump2.jpg Carsten Vilhelmsen, 09/07/2017 01:30 PM
bpi_billeder_dump3.jpg (31.3 KB) bpi_billeder_dump3.jpg Carsten Vilhelmsen, 09/07/2017 01:30 PM
bpi_billeder_dump4.jpg (92.4 KB) bpi_billeder_dump4.jpg Carsten Vilhelmsen, 09/07/2017 01:30 PM
syndiker_billeder.PNG (184 KB) syndiker_billeder.PNG Carsten Vilhelmsen, 09/18/2017 03:55 PM

Related issues

Related to DDB CMS - Enhancement #1476: Fix af BPI's håndtering af billederClosed
Related to DDB CMS - Bug #1678: BPI previewResolved (tag version)

History

#1 Updated by Mikkel Ricky over 3 years ago

Pull request (bpi-modul): https://github.com/ding2/ding2/pull/402
Pull request (bpi-client): https://github.com/ding2/bpi-client/pull/11

Ændringerne til bpi-modulet afhænger af ændringerne til bpi-client.

#2 Updated by Rolf Madsen over 3 years ago

#3 Updated by Rolf Madsen over 3 years ago

  • Status changed from New to Needs code review
  • Assignee set to Gitte Barlach
  • Target version set to DDB CMS 2017 1. opgradering (7.x-4.0.2)

#4 Updated by Gitte Barlach over 3 years ago

  • Assignee changed from Gitte Barlach to Jørgen Nielsen

#5 Updated by Jørgen Nielsen over 3 years ago

  • Status changed from Needs code review to Reviewed - Needs info/rework
  • Assignee changed from Jørgen Nielsen to Mikkel Ricky

Jeg har en enkelt kommentar på bpi-modulet

#6 Updated by Mikkel Ricky over 3 years ago

Jeg har synkroniseret min branch med master-branchen.

Sidst jeg var igennem en tilsvarende øvelse (med rettelser både i bpi-modulet og i klienten) bad Kasper mig om at ændre ding2.make så den pegede på min branch (jf. https://github.com/rimi-itk/ding2/commit/c7c970a2303411811a8f29d96be435e8e795b7c4). Hvad er den rigtige måde at gøre det på?

#7 Updated by Mikkel Ricky over 3 years ago

  • Status changed from Reviewed - Needs info/rework to Needs code review
  • Assignee changed from Mikkel Ricky to Jørgen Nielsen

#8 Updated by Jørgen Nielsen over 3 years ago

Den rigtige måde at gøre det på, er at merge bpi-client koden ind i ding2/bpi-client og tagge en ny version. Og derefter opdatere ding2.make så den peger på den taggede version, og merge ding2. Men jeg kan se, at vi i dag peger på master branch af ding2/bpi-client i ding2.make.

Under udvikling giver det nu god mening at pege på den featurebranch i det repository, som koden har en afhængighed til.

#9 Updated by Mikkel Ricky over 3 years ago

Tak for svaret. Jeg forstår dog ikke helt hvad jeg skal gøre for at I vil acceptere mit pull request på bpi-modulet. Kan du udpensle for mig?

#10 Updated by Jesper Kristensen over 3 years ago

Hvis alt skal være rigtig... så er det kun core der laver tags (fordi tags ikke er en del af et givet PR).

Så alle make files skal pege på ding2 cores repositories (http://github.com/ding2) når der laves et PR, hvilket for et givet PR vil være at de peger på development branchen og ikke et tag, da det næste tag, den version et givet PR skal være med i ikke er givet endnu af core før ved selve releasen.

#11 Updated by Mikkel Ricky over 3 years ago

PR til bpi-modulet afhænger af et PR til bpi-klienten og skal det ikke en eller anden måde fremgå at der er denne sammenhæng?

Givet at der ikke findes development-branches og PR laves til "master", er det så korrekt at der skal stå

libraries[bpi-client][download][url] = "http://github.com/ding2/bpi-client.git"
libraries[bpi-client][download][branch] = "master"

(jf. https://github.com/rimi-itk/ding2/blob/Issue-1999/ding2.make#L434-L435)?

#12 Updated by Gitte Barlach over 3 years ago

Det er rigtig at det skal gøres på den måde.

Det skal frem gå at kommentaren i selv PR over på github og her hvis en eller flere PR's afhænger af hindanden. Så core tydligt kan se/læse at der er flere PR's sammen når de skal merge koden.

#13 Updated by Mikkel Ricky over 3 years ago

Tak for opklaringen. Jeg har tilføjet bemærkninger om afhængigheden både her og på GitHub.

#14 Updated by Jørgen Nielsen about 3 years ago

  • Status changed from Needs code review to Reviewed - Needs info/rework
  • Assignee changed from Jørgen Nielsen to Mikkel Ricky

Der er merge konflikter i BPI client, og Scrutinizer fejl i BPI - kan du rette dem?

#15 Updated by Mikkel Ricky about 3 years ago

Jeg har rettet merge-konflikten i bpi-klienten: https://github.com/ding2/bpi-client/pull/11

#16 Updated by Mikkel Ricky about 3 years ago

Scrutinizer-fejlen i BPI (https://scrutinizer-ci.com/g/ding2/ding2/inspections/20bd319a-c2ed-4f95-b722-34c423504240/issues/files/modules/bpi/bpi.images.inc?status=new&selectedSeverities%5B0%5D=0&selectedAuthors%5B0%5D=rimi%40aarhus.dk&orderField=path&order=asc&fileId=modules%2Fbpi%2Fbpi.images.inc&honorSelectedPaths=0) skyldes at vi ikke har en fastlagt liste over lovlige billedtyper. Derfor bruger jeg t-funktionen med en variabel som argument for at gøre det muligt at lave oversættelser hvis der dukker nye billedtyper op.

#17 Updated by Mikkel Ricky about 3 years ago

  • Assignee changed from Mikkel Ricky to Jørgen Nielsen

#18 Updated by Jørgen Nielsen about 3 years ago

  • Assignee changed from Jørgen Nielsen to Mikkel Ricky

Kan du ikke bruge: t("image_type", array('image_type' => check_plain($image_type))) ?

#19 Updated by Mikkel Ricky about 3 years ago

  • Assignee changed from Mikkel Ricky to Jørgen Nielsen

Jo, men så vil man ikke kunne oversætte den ukendte billedtype. Det er dog nok trods alt en bedre løsning. Jeg retter det. Tak.

#20 Updated by Jørgen Nielsen about 3 years ago

  • Status changed from Reviewed - Needs info/rework to Reviewed
  • Assignee changed from Jørgen Nielsen to Gitte Barlach

reviewet og godkendt

#21 Updated by Kasper Garnæs about 3 years ago

  • Status changed from Reviewed to Technical test

Jeg har merged de to PRs der er nævnt i første kommentar.

#22 Updated by Gitte Barlach almost 3 years ago

  • Assignee changed from Gitte Barlach to Carsten Kaa

#23 Updated by Carsten Vilhelmsen almost 3 years ago

Det er ikke muligt at sende billeder til BPI og det er ikke muligt at hente billeder fra BPI. Testet på upgrade-fbs.ddbcms.dk.
Problemet rammer ikke alle sites, på Tårnbys produktionssite er det således muligt at få adgang til de syndikerede billeder

#24 Updated by Carsten Vilhelmsen almost 3 years ago

  • Status changed from Technical test to Reviewed - Needs info/rework
  • Assignee changed from Carsten Kaa to Mikkel Ricky

#25 Updated by Carsten Vilhelmsen almost 3 years ago

  • Assignee changed from Mikkel Ricky to Carsten Kaa

#26 Updated by Carsten Kaa almost 3 years ago

Har oprettet ny nyhed med titelbillede på upgrade-fbs.ddbcms.dk (som Greve), gemt, redigeret og sendt til BPI med billede: Billedet kommer korrekt med til BPI og indholdet bliver korrekt mærket med billedikon i BPI oversigten.

Ovennævnte indhold delt fra upgrade-fbs (Greve) forsøgte jeg at udgive via BPI til vanilla-fbs (Horsens):

Klik på udgiv i BPI fanebladet ud for indholdet åbner "Opret Nyhed" og henter og udfylder felter fra BPI.
Titelbilledet bliver ikke automatisk sat ind som forventet.

Det var forventet, at der efter klik på "Udgiv" kommer en pop up med visning af de billeder man kan trække ind sammen med nyheden fra BPI.
Denne pop up vises ikke.

Testet i dag kl. 12:51

#27 Updated by Gitte Barlach almost 3 years ago

  • Assignee changed from Carsten Kaa to Anonymous

Kristian, jeg ved ikke om denne skyldes det nye tema. Vil I kigge på det?

Problemet er som Carsten skriver i note #26:

Når man vil udgive indhold fra BPI i sit DDB CMS, bliver Titelbilledet ikke automatisk sat ind som forventet. Det forventes, at der efter klik på "Udgiv" kommer en pop up med visning af de billeder man kan trække ind sammen med nyheden fra BPI.
Denne pop up vises ikke.

#28 Updated by Anonymous over 2 years ago

  • Assignee changed from Anonymous to Anonymous

#29 Updated by Christel Krabbenhøft over 2 years ago

  • Priority changed from Normal to High

#30 Updated by Anonymous over 2 years ago

  • Assignee changed from Anonymous to Gitte Barlach

Pop up fejlen er relateret til #1678

#31 Updated by Gitte Barlach over 2 years ago

  • Status changed from Reviewed - Needs info/rework to Need more info
  • Assignee changed from Gitte Barlach to Anonymous

Hej Phillip

Betyder det af dette issue løses med #1678, så jeg kan lukke sagen?

#32 Updated by Anonymous over 2 years ago

  • Assignee changed from Anonymous to Gitte Barlach

#1678 burde løse problemet med popup vinduet. Jeg har ikke taget stilling til andre kommentar i issuet end fra 26.

#33 Updated by Rolf Madsen over 2 years ago

  • Is duplicate of Bug #1678: BPI preview added

#34 Updated by Rolf Madsen over 2 years ago

  • Status changed from Need more info to Technical test

Det lader til at Kasper, som det fremgår i http://platform.dandigbib.org/issues/1999#note-21, har merget de tidligere PR's i dette issue som bliver behandlet frem til kommentar 26.

Jeg antager dermed at rettelserne til BPI skal testes og at den rettelse Philip omtaler er testet i #1678

#35 Updated by Rolf Madsen over 2 years ago

  • Is duplicate of deleted (Bug #1678: BPI preview)

#36 Updated by Rolf Madsen over 2 years ago

#37 Updated by Carsten Vilhelmsen over 2 years ago

Der er et grundlæggende problem med visning af billeder
Workflow:

a) Artikel med titelbillede, listebillede og billede i brøndtekst uploades til BPI ("testnyhed med billeder")
b) På biblioteket som uploader indholdet, vises artiklen som værende inkl. billede (dump 1)
c) Klikker man på gennemse, vises en dummybillede i stedet for det uploadede billede (dump 2)
d) På biblioteket som vil downloade indholdet, vises artiklen som værende uden tilknyttet billede (dump 3)
e) Klikker man på den downloade artikel, er kun dummybilledet i brødteksten blevet syndikeret (dump 4)

Vi ønsker:
Når artikler har billeder tilknyttet syndikeres, skal de vises med et tilsvarende ikon i BPI-oversigten
Billederne som syndikeres skal være titelbilleder
De syndikerede billeder skal være de samme billeder som uploades, ikke en dummyversion.

#38 Updated by Anonymous over 2 years ago

  • Assignee changed from Anonymous to Carsten Vilhelmsen

Når jeg tester med det commit der er i #1678 ser jeg ikke et billed ikon, men et billede og billedet kommer ikke ind i WYSIWYG editoren, men i billed feltet når jeg trykker ok til at hente det ned.

Jeg har ikke nok indsigt i bpi modulet til at vide om det er relateret til popup delen af dette issue. Måske der er en anden mere kvalificeret der kan give et hurtigt bud?

#39 Updated by Carsten Kaa over 2 years ago

En del af de ting Carsten Vilhelmsen skriver skyldes at BPI servicen URL'en på test sitet ikke er den rigtige.
Jeg tester og kommer tilbage med en afklaring.

#40 Updated by Carsten Kaa over 2 years ago

Den BPI service URL der skal bruges til test af billedfix er: http://bpi1-partial.stg.inlead.dk/

Har lige testet Bug #1678 og kan ikke se at preview er fixed.
Lad os få 1678 fixed først - den griber sandsynligvis også ind i #1999.

#41 Updated by Carsten Vilhelmsen over 2 years ago

  • Assignee changed from Carsten Vilhelmsen to Carsten Kaa

Og lige for fuldstændighedens skyld: tags medtages ikke, når artikler uploades til BPI

#42 Updated by Carsten Kaa over 2 years ago

Noteret! Giv lyd når jeg kan teste.
Kunne godt tænke mig at #1678 er løst først - så er vi sikre på den ikke griber ind i dette issue.

#43 Updated by Rolf Madsen over 2 years ago

  • Status changed from Reviewed - Needs info/rework to Technical test

Jeg har tilladt mig at ændre status til Technical test så vi kan teste den med rettelsen i #1678.

Hvis der er noget jeg har overset så status burde være forblevet Reviewed - needs rework så giv lyd!

#44 Updated by Carsten Kaa over 2 years ago

Jeg har testet på upgrade.fbs og vanilla.fbs - oprettet og sendt indhold til BPI som Greve og lånt som Horsens.
Testet på BPI url: http://bpi1-partial.stg.inlead.dk

Jeg har ikke fundet fejl omkring billedfix.
jeg bliver præsenteret for de billeder der følger med BPI og de sættes ind korrekt.

Tags deles også. Hvis et bibliotek ønsker at dele tags - skal der mappes til tags og sættes et flueben i BPI Indholdstilknytning.

#45 Updated by Rolf Madsen over 2 years ago

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

Det læser jeg som om du kan godkende løsningen?

Sig til hvis jeg har misforstået men ellers ...

Testet og godkendt!

:-)

#46 Updated by Carsten Kaa over 2 years ago

på baggrund af min test kan jeg godt godkende det - men det vil ikke skade at få f.eks. Carsten Bo Vilhelmsen til også at nikke til det :-)

#47 Updated by Carsten Vilhelmsen over 2 years ago

kigger lige :)

#48 Updated by Carsten Vilhelmsen over 2 years ago

  • Assignee changed from Carsten Kaa to Carsten Vilhelmsen

#49 Updated by Carsten Vilhelmsen over 2 years ago

  • Assignee changed from Carsten Vilhelmsen to Carsten Kaa

@Carsten: kan du sende en nyhed med titel/liste/brødtekst billeder + tags fra Århus? Jeg kan ikke teste syndikeringen af en artikel fra Greve på stagingsitet, da vi deler det samme agency

#50 Updated by Carsten Kaa over 2 years ago

jeg har sat BPI op på de to test sites:
upgrade.fbs (Greve) og vanilla.fbs (Horsens)
Så kan du teste om det du deler fra Greve virker i Horsens.
Hvis vi vil ændre biblioteker skal vi ind og ændre agency keys i BPI indstillinger - sig til hvis du mener det er nødvendigt.

#51 Updated by Carsten Vilhelmsen over 2 years ago

Gentests: Upgrade-fbs oploader artikel med liste, titel og indholdsbillede samt tags. Vanilla-fbs syndikerer artiklen + indhold. Den samme proces gentages i modsat rækkefølge. I preview vises tekst + indholdsbillede, men ikke tags.
Testet og godkendt.
Dog savnes en mulighed for at fravælge upload af tags.

#52 Updated by Carsten Vilhelmsen over 2 years ago

  • Status changed from Reviewed - Needs info/rework to Resolved (tag version)

#53 Updated by Carsten Vilhelmsen about 2 years ago

  • Description updated (diff)

Dummyproblemet, som nævnes i https://platform.dandigbib.org/issues/1999#note-37 er endnu ikke løst.

#54 Updated by Rolf Madsen about 2 years ago

  • Assignee changed from Carsten Kaa to Carsten Vilhelmsen

@Carsten, kan jeg få dig til at opsummere det ikke løste problem i et nyt issue?

Så skal jeg sørge for den blive ekspederet videre!

#55 Updated by Carsten Vilhelmsen about 2 years ago

  • Related to Bug #3574: BPI. Billeder overføres ikke til BPI added

#56 Updated by Rolf Madsen over 1 year ago

  • Related to deleted (Bug #3574: BPI. Billeder overføres ikke til BPI)

Also available in: Atom PDF