Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FINNA-2662] Adjust loan history export to use paging #3076

Open
wants to merge 25 commits into
base: dev
Choose a base branch
from

Conversation

LuomaJuha
Copy link

@LuomaJuha LuomaJuha commented Nov 4, 2024

Adjustments:

  • Display max amount of pages to download from, select from which page to start download.
  • Use post
  • Download limits now configurable

TODO:

  • Translations checked

@LuomaJuha LuomaJuha requested review from EreMaijala and removed request for EreMaijala November 4, 2024 14:51
@LuomaJuha
Copy link
Author

Small adjustment to allow more large paging.

Copy link

@EreMaijala EreMaijala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Jos sivutus ei ole käytössä, paginator on false ja downloadhistory.phtml rivi 7 kaatuu
  • Pitäisikö sivujen määrä olla jotenkin suhteessa sivun kokoon? Tyyliin ceil(1000 / sivun pituus)?
  • Lomakkeen toimintaa pitää vielä miettiä. Ehkä olisi parempi, että siinä olisi vaikka checkbox, jolla valitaan, lisätäänkö olemassaolevaan tiedostoon. Nykyinen ei ole kovin ymmärrettävä. Ei se ihan selväksi tulisi sittenkään, kun lopullisen tiedoston tallennuksessa tulee alkuperäinen yliajettavaksi ja siitä tietysti varoitus.
  • Toisaalta en tykkää ajatuksesta, että käyttäjä pystyy lataamaan palvelimelle jonkin sattumanvaraisen tiedoston, jota sitten koitetaan parsia. Tässä on aika merkittävä hyökkäysvektori. Mitä jos tehtäisiin nyt vähän yksinkertaisempana niin, että voi vain tallentaa uuteen tiedostoon tietyt sivut? Jää toki enemmän työtä käyttäjälle, mutta on sekin parempi kuin epäonnistuva vienti.

local/languages/finna/fi.ini Outdated Show resolved Hide resolved
local/languages/finna/fi.ini Outdated Show resolved Hide resolved
themes/finna2/templates/checkouts/downloadhistory.phtml Outdated Show resolved Hide resolved
@EreMaijala
Copy link

@LuomaJuha Perhaps just avoid the modal and display the download options on a separate page?

@LuomaJuha
Copy link
Author

@EreMaijala joo, yksinkertaistan ja poistan tuon tiedostoon jatkamisen. Voi tulla muuten jotain ylimäärästä säätöä tuosta.

@EreMaijala
Copy link

Tuli mieleen jännä ajatus tuosta lainaushistorian lataamisesta: Mitä jos tekisikin sellaisen, että kun ladattavaa on liikaa, näytetään nappula "Lataa osa 1/5". Kun sitä klikkaa, se pistää lataukseen ekan osan ja vaihtaa nappulaksi "Lataa osa 2/5"? Sitten se voisi lisätä tiedoston nimeen myös osan numeron fiksusti, ja lataaminen olisi niin helppoa kuin tällä systeemillä vaan voi.

@LuomaJuha LuomaJuha removed the request for review from EreMaijala November 19, 2024 08:15
@LuomaJuha
Copy link
Author

@EreMaijala Poistan review:n ja laitan uudestaan kun on tuunattu

@LuomaJuha LuomaJuha requested a review from EreMaijala January 13, 2025 11:00
local/languages/finna/en-gb.ini Outdated Show resolved Hide resolved
module/Finna/config/module.config.php Outdated Show resolved Hide resolved
module/Finna/src/Finna/Controller/AjaxController.php Outdated Show resolved Hide resolved
@EreMaijala
Copy link

Lataus tarttee progress-indikaattorin. Nyt ei näy mitenkään, jos jotain tapahtuu.

@@ -311,6 +311,10 @@ library_cards = true
; memory problems for users with a large number of historic loans). Default = 50
;historic_loan_page_size = 50

; Limit for how many historic transactions to fetch at most from ILS
; when downloading loan history
loan_history_download_batch_limit = 1000

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tämä ei näytä vaikuttavan dropdownissa näkyvään sivumäärään.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ny pitäis taas vaikuttaa!

@LuomaJuha LuomaJuha requested a review from EreMaijala January 15, 2025 17:48
Copy link

@EreMaijala EreMaijala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Jotain hämärää tossa on edelleen on noiden sivutusten kanssa. Jos asetuksissa on

[Catalog]
historic_loan_page_size = 3
loan_history_download_batch_limit = 2

Niin jos on 7 lainaa, kertoo oikein, että neljä sivua. Mutta kun laittaa latauksen menemään, niin saa tiedostot:

  1. -1-1, jossa on 3 tietuetta
  2. -2-2, jossa on toiset 3
  3. -3-3, jossa on 1 tietue
  4. -4-4, jossa ei ole mitään

Pitäisikö ton jälkimmäisen numeron tiedoston nimessä olla kokonaismäärä?

@LuomaJuha
Copy link
Author

Hmm, pitääkin katsoa mitäs siel tapahtuu. Toi vika oli ns viimeinen sivu. Veikkaan että jätän sen pois jos on vaan 1 sivu tuloksessa.

@LuomaJuha LuomaJuha requested a review from EreMaijala January 16, 2025 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants