Resolve "Pdf-merge lisäykset" - [merged] - Githubissues

TIM-JYU / TIM

TIM (The Interactive Material) is an open-source cloud-based platform for creating interactive learning documents.

https://tim.education/view/about/en-US

MIT License

14 stars 4 forks source link

Resolve "Pdf-merge lisäykset" - [merged] #2685

Closed dezhidki closed 2 years ago

dezhidki commented 5 years ago

In GitLab by @vivanauk on Sep 20, 2019, 13:26

Merges 1533-pdf-merge -> master

Closes #1533

dezhidki commented 5 years ago

In GitLab by @vivanauk on Sep 20, 2019, 13:35

added 1 commit

ed225d20 - Add comments and rename functions

Compare with previous version

dezhidki commented 5 years ago

In GitLab by @vivanauk on Sep 20, 2019, 14:44

added 1 commit

434e3c84 - Add opening selectively merged documents and remove redundant routes

Compare with previous version

dezhidki commented 5 years ago

In GitLab by @vivanauk on Sep 20, 2019, 14:46

added 1 commit

5ade669b - Use tmp-folder for merged files

Compare with previous version

dezhidki commented 5 years ago

In GitLab by @vivanauk on Sep 23, 2019, 13:25

added 1 commit

51e88590 - Add shortening PDFtk error messages and log unexpected errors

Compare with previous version

dezhidki commented 5 years ago

In GitLab by @vivanauk on Sep 23, 2019, 13:26

added 1 commit

e565fb95 - Add disabling erroneous attachment selection

Compare with previous version

dezhidki commented 5 years ago

In GitLab by @vivanauk on Sep 23, 2019, 14:15

added 1 commit

b2e66fdd - Add better and more concise error messages

Compare with previous version

dezhidki commented 5 years ago

In GitLab by @vivanauk on Sep 23, 2019, 14:22

added 1 commit

c1af470b - Add links to attachments to merge

Compare with previous version

dezhidki commented 5 years ago

In GitLab by @vivanauk on Sep 23, 2019, 14:43

added 1 commit

8a00850f - Add scrolling and move buttons to footer

Compare with previous version

dezhidki commented 5 years ago

In GitLab by @vivanauk on Sep 23, 2019, 15:13

added 1 commit

fed2a12a - Add creating merged file into a document specific folder

Compare with previous version

dezhidki commented 5 years ago

In GitLab by @vivanauk on Sep 23, 2019, 15:14

added 1 commit

12cb8b36 - Fix merged file link pushing buttons aside

Compare with previous version

dezhidki commented 5 years ago

In GitLab by @vivanauk on Sep 23, 2019, 15:14

added 21 commits

12cb8b36...d5c41ecd - 9 commits from branch master
952c498f - Add selecting attachments to merge
4238686d - Add showing only attachment file names and remove list bullets
f94082e6 - Add comments and rename functions
015d8661 - Add opening selectively merged documents and remove redundant routes
b9b7bb18 - Use tmp-folder for merged files
1459211a - Add shortening PDFtk error messages and log unexpected errors
cfa7d55f - Add disabling erroneous attachment selection
63fc5def - Add better and more concise error messages
b8a201f6 - Add links to attachments to merge
491f1a31 - Add scrolling and move buttons to footer
89dc2bdb - Add creating merged file into a document specific folder
ba6b32e9 - Fix merged file link pushing buttons aside

Compare with previous version

dezhidki commented 5 years ago

In GitLab by @vivanauk on Sep 24, 2019, 12:56

added 1 commit

1c42535a - Add support for TIM-URLs when merging attachments

Compare with previous version

dezhidki commented 5 years ago

In GitLab by @vivanauk on Sep 24, 2019, 12:57

added 1 commit

bee1e3e0 - Add support for TIM-URLs when merging attachments

Compare with previous version

dezhidki commented 5 years ago

In GitLab by @vivanauk on Sep 24, 2019, 13:08

added 1 commit

e532d406 - Add hash and better error messages

Compare with previous version

dezhidki commented 5 years ago

In GitLab by @Smibu on Sep 24, 2019, 13:40

Commented on timApp/document/minutes/routes.py line 271

Path-oliolla on mkdir-metodi, eli ei pitäisi tarvita os.mkdir.

dezhidki commented 5 years ago

In GitLab by @Smibu on Sep 24, 2019, 13:40

Commented on timApp/document/minutes/routes.py line 268

Mieluummin suoraan short_namea:

doc_name = d.short_name

dezhidki commented 5 years ago

In GitLab by @Smibu on Sep 24, 2019, 13:40

Commented on timApp/document/minutes/routes.py line 272

Noita importteja voisi muuttaa niin, ettei tarvitse toistella tuota timApp.util.pdftools..

dezhidki commented 5 years ago

In GitLab by @Smibu on Sep 24, 2019, 13:40

Commented on timApp/document/minutes/routes.py line 281

Tähän suora URL mukaan, jotta selaimessa ei tarvitse mitään tyyliin path.replace("tim_files/blocks/", "").

dezhidki commented 5 years ago

In GitLab by @Smibu on Sep 24, 2019, 13:40

Commented on timApp/static/scripts/tim/document/minutes/mergePdfCtrl.ts line 137

Tätä linkinmuodostuslogiikkaa ei saisi selainpäässä tarvita vaan palvelimen pitäisi antaa suora oikea URL.

dezhidki commented 5 years ago

In GitLab by @Smibu on Sep 24, 2019, 13:40

Commented on timApp/document/minutes/routes.py line 293

Tähän TODO, että PDF luodaan lennosta (kunhan on edit-oikeus).

dezhidki commented 5 years ago

In GitLab by @Smibu on Sep 24, 2019, 13:40

Commented on timApp/document/minutes/routes.py line 294

Riittääkö view? cc @vesal saako view-oikeudella päästä lukemaan pöytäkirjojen liitteitä? Vai miten se meni?

dezhidki commented 5 years ago

In GitLab by @vivanauk on Sep 24, 2019, 14:10

Commented on timApp/static/scripts/tim/document/minutes/mergePdfCtrl.ts line 137

Siinä joutuu sitten lisäämään poistetun osan takaisin mergeä tehtäessä (tai erottamaan klikattavan linkin ja mergettävän tiedostopolun), sillä lyhennettyä versiota ei käytetä muualla kuin liite-listan linkeissä.

dezhidki commented 5 years ago

In GitLab by @vivanauk on Sep 24, 2019, 14:18

Commented on timApp/document/minutes/routes.py line 271

changed this line in version 16 of the diff

dezhidki commented 5 years ago

In GitLab by @vivanauk on Sep 24, 2019, 14:18

Commented on timApp/document/minutes/routes.py line 268

changed this line in version 16 of the diff

dezhidki commented 5 years ago

In GitLab by @vivanauk on Sep 24, 2019, 14:18

Commented on timApp/document/minutes/routes.py line 272

changed this line in version 16 of the diff

dezhidki commented 5 years ago

In GitLab by @vivanauk on Sep 24, 2019, 14:18

Commented on timApp/document/minutes/routes.py line 281

changed this line in version 16 of the diff

dezhidki commented 5 years ago

In GitLab by @vivanauk on Sep 24, 2019, 14:18

Commented on timApp/static/scripts/tim/document/minutes/mergePdfCtrl.ts line 137

changed this line in version 16 of the diff

dezhidki commented 5 years ago

In GitLab by @vivanauk on Sep 24, 2019, 14:18

added 1 commit

f40d3e36 - Address review comments

Compare with previous version

dezhidki commented 5 years ago

In GitLab by @Smibu on Sep 25, 2019, 09:59

Commented on timApp/document/minutes/routes.py line 294

@vivanauk Laita tähän varulta edit, koska masterin send_file vaatii edit-oikeuden, niin pysyy ainakin samana.

dezhidki commented 5 years ago

In GitLab by @Smibu on Sep 25, 2019, 09:59

Commented on timApp/document/minutes/routes.py line 281

Tässä on itse asiassa polkuinjektion vaara, koska jos file_filename on tyyliin ../../../../joku_tiedosto, niin tuon avulla pääsee mihin tahansa tiedostoon käsiksi.

Tämä täytyy muuttaa niin, että URLiin tulee parametreina (ei siis polkuosassa) doc_id ja lista tiedostonimistä. Eli tässä GET-reitissä olisi sama use_args kuin POSTissa (ja modelissa doc_pathin voisi vaihtaa doc_id:ksi). Muutenkin tuo lennosta luominen vaatii sen, että lista tiedostonimistä tulee myös GETille.

Sitten tämä reitti laskee hashin samalla tavalla kuin POST ja katsoo sen perusteella, onko mergetty tiedosto olemassa.

dezhidki commented 5 years ago

In GitLab by @Smibu on Sep 25, 2019, 09:59

Commented on timApp/document/minutes/routes.py line 277

Tässä URLin muodostukseen voi käyttää urlencode-funktiota (ks. GET-reitin kommentti).

dezhidki commented 5 years ago

In GitLab by @Smibu on Sep 25, 2019, 10:19

Commented on timApp/document/minutes/routes.py line 243

Tämä pitäisi nimetä urls (ja lokaalit muuttujat vastaavasti), koska nämä annetaan ennen pitkää tuonne parse_tim_url.

dezhidki commented 5 years ago

In GitLab by @Smibu on Sep 25, 2019, 10:19

Commented on timApp/util/pdftools.py line 388

Tästä puuttuu f alusta ja URLista /, jolloin se on sama kuin edellinen if.

dezhidki commented 5 years ago

In GitLab by @Smibu on Sep 25, 2019, 10:19

Commented on timApp/document/minutes/routes.py line 273

Tästä puuttuu tiedostojen oikeustarkistukset.

Lisää UploadedFile-luokkaan staattinen get_by_url-metodi, joka palauttaa UploadedFile-olion URLin perusteella, jos sellainen löytyy. Käytännössä ongi URLista se tiedoston id ja kutsu UploadedFile.find_by_id_and_type.

Sitten jokaiselle UploadedFile-oliolle voi kutsua verify_view_access(uf.block, check_parents=True).

merge_pdfs saisi listan UploadedFile-olioita.

dezhidki commented 5 years ago

In GitLab by @vivanauk on Sep 27, 2019, 10:33

added 35 commits

f40d3e36...63504267 - 20 commits from branch master
6ad78dd8 - Add selecting attachments to merge
4f428c17 - Add showing only attachment file names and remove list bullets
36380fb1 - Add comments and rename functions
6bd817d3 - Add opening selectively merged documents and remove redundant routes
f076bf43 - Use tmp-folder for merged files
9a53d840 - Add shortening PDFtk error messages and log unexpected errors
bef0746d - Add disabling erroneous attachment selection
4ba5fc40 - Add better and more concise error messages
b2b537fe - Add links to attachments to merge
eca8b44a - Add scrolling and move buttons to footer
e808ce9a - Add creating merged file into a document specific folder
2f099b74 - Fix merged file link pushing buttons aside
697dd329 - Add support for TIM-URLs when merging attachments
0847b703 - Add hash and better error messages
bc5aade8 - Address review comments

Compare with previous version

dezhidki commented 5 years ago

In GitLab by @vivanauk on Sep 27, 2019, 10:42

Commented on timApp/document/minutes/routes.py line 281

Miten jos käyttäjä muuttaa valittuja liitteitä ennen mergetyn tiedoston avaamista? Tallennetaanko vain edellisen mergen path-lista muistiin mergetyn tiedoston avaamisreittiä varten?

dezhidki commented 5 years ago

In GitLab by @vesal on Sep 27, 2019, 10:46

Commented on timApp/document/minutes/routes.py line 281

+@minutes_blueprint.route('/openMergedAttachment//')

Miten jos käyttäjä muuttaa valittuja liitteitä ennen mergetyn tiedoston avaamista? Tallennetaanko vain edellisen mergen path-lista muistiin mergetyn tiedoston avaamisreittiä varten?

Tarkoitatko että jos teke mergen, mutta sitten muuttaa tiedostoja ja vasta sitten katsoo mergen tulokset. Jos noin, niin oma vika :-)

Vesa

dezhidki commented 5 years ago

In GitLab by @Smibu on Sep 27, 2019, 10:50

Commented on timApp/document/minutes/routes.py line 281

Visa kai tarkoittaa, että jos muuttaa rukseja mergen jälkeen, eli että tiedostolista vaihtuu.

Minusta sen mergen path-listan voi tosiaan tallentaa johonkin kontrollerin attribuuttiin, ja antaa sitten GETille sen.

dezhidki commented 5 years ago

In GitLab by @Smibu on Sep 27, 2019, 10:55

Commented on timApp/document/minutes/routes.py line 281

Tai tallentaa suoraan sen URLin, jonka POST palauttaa (joka sitten sisältää tarvittavat parametrit).

dezhidki commented 5 years ago

In GitLab by @vivanauk on Sep 27, 2019, 11:09

Commented on timApp/document/minutes/routes.py line 281

Aiemmin oli suoraan linkki GET-reitille jota kautta pdf aukesi selaimeen, mutta miten sama toiminnallisuus saadaan POST-metodilla?

Ilmeisesti linkin muuttaminen toimimaan POSTilla ei ole aivan yksinkertaista, joten vaihdoin sen funktioon, mutta mikä on oikeaoppinen tapa avata saatu tiedosto, kun se on result.datan sisällä?

dezhidki commented 5 years ago

In GitLab by @Smibu on Sep 27, 2019, 11:25

Commented on timApp/document/minutes/routes.py line 281

Aiemmin oli suoraan linkki GET-reitille jota kautta pdf aukesi selaimeen, mutta miten sama toiminnallisuus saadaan POST-metodilla?

Olen jonkun kohdan selittänyt epätarkasti, koska tarkoitus on toki edelleen, että GET-reitti avaa PDF:n.

Eli POST-reitti generoi mergetyn PDF:n ja palauttaa JSONin tyyliin:

{"url": "/openMergedAttachment?doc_id=12345&files%5B%5D='Reference to deleted milestone 2'Ffiles%2F123%2Feka.pdf&files%5B%5D='Reference to deleted milestone 2'Ffiles%2F123%2Ftoka.pdf&files%5B%5D='Reference to deleted milestone 2'Ffiles%2F123%2Fkolmas.pdf"}

jossa tuo esimerkin parametrilista on saatu Pythonilla:

urlencode({'doc_id': 12345, 'files[]': ["/files/123/eka.pdf", "/files/123/toka.pdf", "/files/123/kolmas.pdf"]}, doseq=True)

Eli se use_args osaa kaivaa parametrit sekä POST että GET tapauksessa, eli siksi sen pitäisi toimia kummassakin.

dezhidki commented 5 years ago

In GitLab by @Smibu on Sep 27, 2019, 11:27

Commented on timApp/document/minutes/routes.py line 281

Tuossa esimerkissä tiedostojen id:t pitäisi toki olla eriävät, nyt ne on kaikki 123.

dezhidki commented 5 years ago

In GitLab by @vivanauk on Sep 27, 2019, 11:56

Commented on timApp/document/minutes/routes.py line 281

Jos laittaa urlencode({'doc_id': 12345, 'files[]':...) niin open reitti valittaa ettei löydy urls ({'urls': ['Missing data for required field.']}) eli halunnee samassa muodossa kuin Model on.

Toisaalta jos laitan niin, tällöin samalla datalla use_args antaa POST-reitissä (merge):

['/files/34/testikurssi-2_stamped.pdf', '/files/35/testikurssi-2.pdf']

ja GET-reitissä (open merged file):

["['/files/34/testikurssi-2_stamped.pdf', '/files/35/testikurssi-2.pdf']"]

eli siinä on eroa toiminnassa POST ja GET välillä.

dezhidki commented 5 years ago

In GitLab by @Smibu on Sep 27, 2019, 12:01

Commented on timApp/document/minutes/routes.py line 281

open reitti valittaa ettei löydy urls

Joo tosiaan, nimi pitää olla urls ja se on muutenkin parempi kuin files.

eli siinä on pieni ero POST ja GET välillä.

Hmm... mitä jos kokeilet urlencodessa eri variaatioita, esim. urls eikä urls[], ja vaikka doseq=False. Vaikuttaako ne mitään?

dezhidki commented 5 years ago

In GitLab by @Smibu on Sep 27, 2019, 12:04

Commented on timApp/document/minutes/routes.py line 281

Tuo ohje: https://webargs.readthedocs.io/en/latest/quickstart.html#parsing-lists-in-query-strings

ainakin viittaisi siihen, että noita [] ei pidä olla siinä urlencodessa.

dezhidki commented 5 years ago

In GitLab by @vivanauk on Sep 27, 2019, 12:06

Commented on timApp/document/minutes/routes.py line 281

Tämä näyttäisi toimivan: urlencode({'doc_id': doc_id, 'urls': pdf_urls}, doseq=True).

dezhidki commented 5 years ago

In GitLab by @Smibu on Sep 27, 2019, 12:07

Commented on timApp/document/minutes/routes.py line 281

Okei joo, hyvä.

dezhidki commented 5 years ago

In GitLab by @vivanauk on Sep 27, 2019, 12:30

Commented on timApp/document/minutes/routes.py line 273

Miten muuten se halutaan, kun saman id:n takana on leimattujen liitteiden tapauksessa sekä perus-upload että _stamped versio? Pelkän id:n perusteella ei voi tietää kumpi on käytössä, mutta katsonko vain URListä vielä onko sen osana _stamped.pdf?

Ja jos tuo metodi tulee muuhunkin käyttöön, niin meneekö mahdollisesti UploadedFile.find_by_id_and_type varten tarvittava tyyppi pieleen, kun vaihtoehtoja ovat esim. image, file ja upload?

dezhidki commented 5 years ago

In GitLab by @Smibu on Sep 27, 2019, 12:38

Commented on timApp/document/minutes/routes.py line 273

Tuolla timApp/upload/upload.py:ssä on reitti get_file, jossa hoidetaan tuo stamped-käsittely. Irrota siitä funktio, jolle menee parametreina (file_id: int, filename: str) ja joka palauttaa joko UploadedFile tai StampedPDF-olion. Sitten voit kutsua sitä.

dezhidki commented 5 years ago

In GitLab by @vivanauk on Sep 27, 2019, 13:11

Commented on timApp/document/minutes/routes.py line 273

funktio, jolle menee parametreina (file_id: int, filename: str)

Laitoin samoin tuon staattisena UploadedFile sisään.

Next