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.

Sign up for GitHub

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

fix usage of kundensystemId #471

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

fix usage of kundensystemId #471

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7cd3ddd
fix usage of kundensystemId
nemiah Jan 6, 2025
03f1b34
fix indentation
nemiah Jan 6, 2025
515f9b5
fix indentation
nemiah Jan 6, 2025
5063003
fix description
nemiah Jan 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions lib/Fhp/FinTs.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -589,6 +589,8 @@ public function selectTanMode($tanMode, $tanMedium = null)
}
$this->selectedTanMode = $tanMode instanceof TanMode ? $tanMode->getId() : $tanMode;
$this->selectedTanMedium = $tanMedium instanceof TanMedium ? $tanMedium->getName() : $tanMedium;

$this->kundensystemId = $this->options->kundensystemId;
Copy link
Contributor

Choose a reason for hiding this comment

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

Wrong indentation (also in the other file), please run the auto-formatter (cs-fix?)

Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this happen within selectTanMode() and not in the same place where the persist() stuff is read back?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Because it works here, no other reason 🤷‍♀️

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd move it into the new() function around line 88, and either put it into the else branch (i.e. only run it when $persistedInstance === null) or into an if ($this->options->kundensystemId === null).

(Without that if protection, it will break any user that doesn't populate the field, because it will overwrite the restored kundensystemId with null -- in fact I'm surprised no tests are breaking.)

}

/**
Expand Down
8 changes: 8 additions & 0 deletions lib/Fhp/Options/FinTsOptions.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,14 @@ class FinTsOptions
/** @var int */
public $timeoutResponse = 30;

/**
* The Kundensystem-Id as returned by the bank and persisted by the application code
* Prevents having to re-authenticate every time on login
* Use DialogInitialization::getKundensystemId() on the return-object of FinTs::login(), to get the new one
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: "return object" without dash (compound nouns in English don't use dash)

My actual question: Why does it say "the new one"? Isn't that the "only" one?

I see two potential misunderstandings (of a library user) here:

  • Am I supposed to run login() every time and then get the kundensystemId?
  • After getting the kundensystemId, do I need to put it into FinTsOptions right away? And then what, create a new FinTs object right away?

Probably it would help to explain how this interacts with persistence, namely that it's meant to be stored in the database and be used the next time a FinTs instance is created, in a new PHP run. And that it's redundant if full persist() is used.

* @var string
Copy link
Contributor

Choose a reason for hiding this comment

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

Make it clear by the type here and in the explanation above that this is optional. And give it a null default value.

*/
public $kundensystemId;

/**
* @throws \InvalidArgumentException If the options are invalid.
*/
Expand Down