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-2235] Quria: Add support for multiple emails/phone numbers #3022

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

Conversation

rajaro
Copy link

@rajaro rajaro commented Sep 11, 2024

No description provided.

@rajaro rajaro requested a review from EreMaijala September 11, 2024 12:28
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.

Tämä lienee muuten ok, mutta ei voida lähteä siitä, että id:t välitetään käyttöliittymän kautta ILS-ajurille, koska silloin kuka vain voi lähettää minkä tahansa ID:n. Pitää käsitellä jotenkin niin, ettei ID näy.

@rajaro rajaro requested a review from EreMaijala October 8, 2024 13:43
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.

Tässä olisi varmaan parempi käyttää yhteystietojen muutoslomaketta (ILS-ajurin updateAddress, MyRearchController::changeProfileAddressAction) . Silloin kaikki menisi samalla lomakkeella ja varsinainen päivityslogiikka pysyisi ILS-ajurin sisäisenä.

module/Finna/src/Finna/ILS/Driver/Quria.php Outdated Show resolved Hide resolved
module/Finna/src/Finna/Controller/MyResearchController.php Outdated Show resolved Hide resolved
module/Finna/src/Finna/Controller/MyResearchController.php Outdated Show resolved Hide resolved
module/Finna/src/Finna/Controller/MyResearchController.php Outdated Show resolved Hide resolved
module/Finna/src/Finna/Controller/MyResearchController.php Outdated Show resolved Hide resolved
@rajaro
Copy link
Author

rajaro commented Dec 10, 2024

Noniin, pienen veivaamisen jälkeen päädyttiin seuraavaan ratkaisuun:

  • Listataan sähköpostit/numerot omat tiedot sivulla
  • Muutokset lomakkeen kautta, jossa jokaisen osoitteen/numeron kohdalla täppä aktivointia varten
  • Mahdollisuus asetuksissa määrittää, onko aktivointi mahdollista vai ei
  • Ei tehdä erikseen kenttiä lisäykselle/poistolle, koska Qurian rajapinta ei tue spostin poistoa

@rajaro rajaro requested a review from EreMaijala December 10, 2024 15:55
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.

Jaron kanssa keskusteltu. Muokataan vielä käyttämään enemmän olemassaolevaa toiminnallisuutta.

@rajaro rajaro requested a review from EreMaijala December 12, 2024 12:08
@@ -882,6 +883,7 @@ Performers = "Performers"
Performing Ensembles = "Performing Ensembles"
Personal details maintained by the library = "Personal details maintained by the library"
Phone = "Phone"
phone_active = "Use for SMS"

Choose a reason for hiding this comment

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

Voisiko käännös vastata paremmin merkitystä tyyliin phone_use_for_sms?

*
* @throws ILSException
*
* @return array Associative array of the results
*/
public function updateEmail($patron, $email)
public function updateEmail($patron, $email, $emailId = null, $active = false)

Choose a reason for hiding this comment

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

Pitäisikö tämän olla nyt protected? Ja ehkä vähän eri nimellä (updateEmailAddress?), ettei sekoitu tuohon vanhaan julkiseen metodiin?

Choose a reason for hiding this comment

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

Sama tietty updatePhone:n kohdalla (protected function updatePhoneNumber?)

@@ -181,6 +181,15 @@
<div class="my-profile-col profile-title"><?=$this->transEsc('Phone') ?>:</div>
<div class="profile-text-value"><?=$this->escapeHtml($this->profile['phone']) ?></div>
<?php endif; ?>
<?php elseif (!empty($this->profile['phone_0'])): ?>

Choose a reason for hiding this comment

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

Voisiko tätä ja email_0:aa vielä välttää mappaamalla phone_0:n myös phone-kenttään ja email_0:n myös email-kenttään? Tai oikeastaan ensimmäisen aktiivisen arvon, se kai voi olla muukin kuin indeksillä 0?

@rajaro rajaro requested a review from EreMaijala January 8, 2025 14:57
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