OBG-442 Date formatting problem solved and deny consent redirects done #605

electronicpeter · 2020-04-22T12:53:08Z

No description provided.

valb3r · 2020-04-22T13:44:44Z

fintech-examples/fintech-ui/src/app/services/storage.service.ts

@@ -61,7 +62,7 @@ export class StorageService {
      return null;
    }
    const date =  new Date(parseInt(validUntilTimestamp));
-    if (date.toLocaleString() === "Invalid Date") {
+    if (Consts.toLocaleString(date) === "Invalid Date") {


Date validity can be checked via:

if(isNaN(date)) { // handle invalid date }

This is better. Thx.

const date = new Date(parseInt(validUntilTimestamp));
if (isNaN(date)) {
return null;
}
Argument of type date is not assignable to parameter of type number.

The weired thing is, that sometimes I got a number, though the localStorage field was empty. And that resulted in a Invalid Date. Unfortunatly I can not reproduce error.

valb3r · 2020-04-22T13:51:35Z

fintech-examples/fintech-ui/src/app/common/navbar/navbar.component.ts

@@ -29,7 +30,7 @@ export class NavbarComponent implements OnInit {
  getSessionValidUntil(): string {
    const validUntilDate: Date = this.storageService.getValidUntilDate();
    if (validUntilDate != null) {
-      const validUntilDateString = validUntilDate.toLocaleString();
+      const validUntilDateString = Consts.toLocaleString(validUntilDate);


The entire regex-part of function can be simplified to:

var time = date.toLocaleTimeString('en-GB')

Yes, but I have several places where this is used. I definitly want to have DateFormating handled in one place. Specially, toLoclaTimeString has a second Parameter, we do not use yet.

You can create utility function and use it everywhere

I put uitilty function in consts.ts. It is no more bound to class Consts.

codecov-io · 2020-04-23T09:41:00Z

Codecov Report

Merging #605 into develop will decrease coverage by 0.42%.
The diff coverage is 41.93%.

@@              Coverage Diff              @@
##             develop     #605      +/-   ##
=============================================
- Coverage      70.83%   70.41%   -0.43%     
  Complexity       673      673              
=============================================
  Files            331      332       +1     
  Lines           3786     3833      +47     
  Branches         352      357       +5     
=============================================
+ Hits            2682     2699      +17     
- Misses           905      935      +30     
  Partials         199      199

Impacted Files	Coverage Δ	Complexity Δ
...impl/controller/FinTechAccountInformationImpl.java	`45.45% <0.00%> (-10.11%)`	`3.00 <0.00> (ø)`
...sys/opba/fintech/impl/tppclients/HeaderFields.java	`0.00% <ø> (ø)`	`0.00 <0.00> (ø)`
...c/app/api/api/finTechAccountInformation.service.ts	`40.00% <ø> (ø)`	`0.00 <0.00> (ø)`
...ui/src/app/api/api/finTechAuthorization.service.ts	`25.00% <0.00%> (-0.76%)`	`0.00 <0.00> (ø)`
...ch-ui/src/app/api/api/finTechBankSearch.service.ts	`32.14% <ø> (ø)`	`0.00 <0.00> (ø)`
...les/fintech-ui/src/app/api/model/accountDetails.ts	`100.00% <ø> (ø)`	`0.00 <0.00> (ø)`
.../app/bank/list-accounts/list-accounts.component.ts	`37.50% <0.00%> (-1.64%)`	`0.00 <0.00> (ø)`
...app/bank/services/consent-authorization.service.ts	`9.09% <0.00%> (ø)`	`0.00 <0.00> (?)`
...ntech-ui/src/app/common/navbar/navbar.component.ts	`21.42% <0.00%> (ø)`	`0.00 <0.00> (ø)`
...amples/fintech-ui/src/app/login/login.component.ts	`11.11% <ø> (ø)`	`0.00 <0.00> (ø)`
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5885a33...d2282b8. Read the comment docs.

valb3r · 2020-04-23T10:16:29Z

...ech-impl/src/main/java/de/adorsys/opba/fintech/impl/controller/FinTechAuthorizationImpl.java

@@ -58,6 +58,11 @@

    @Override
    public ResponseEntity fromConsentOkGET(String authId, String finTechRedirectCode, UUID xRequestID, String xsrfToken) {
+        if (!authorizeService.isAuthorized()) {
+            log.warn("fromConsentOkGET failed: user is not authorized! - path is \"/v1/" + authId + "/fromConsentOk\"");


Can be done without string concat:
log.warn("fromConsentOkGET failed: user is not authorized! - path is \"/v1/{}/fromConsentOk\"", authId);

valb3r · 2020-04-23T10:17:56Z

fintech-examples/fintech-ui/src/app/common/navbar/navbar.component.ts

@@ -29,7 +30,7 @@ export class NavbarComponent implements OnInit {
  getSessionValidUntil(): string {
    const validUntilDate: Date = this.storageService.getValidUntilDate();
    if (validUntilDate != null) {
-      const validUntilDateString = validUntilDate.toLocaleString();
+      const validUntilDateString = Consts.toLocaleString(validUntilDate);


You can create utility function and use it everywhere

elroman · 2020-04-27T09:47:33Z

...ech-impl/src/main/java/de/adorsys/opba/fintech/impl/controller/FinTechAuthorizationImpl.java

+            return new ResponseEntity<>(HttpStatus.UNAUTHORIZED);
+        }
+
+        if ("ok".equalsIgnoreCase(okOrNotok) && consentService.confirmConsent(authId, xRequestID)) {


as I understood okOrNotok is a state of some session. And in this case, the boolean type will be enough in order to recognize 'ok' (true) or notOk (false) state.
But maybe using an enum as an indicator of state will be preferred. Enum value could be compared with the state without hardcoded string value.

Ok, changed it to enum in api, impl and ui. But api enum is not generated, to I had to create enum value by hand (OpenAPITools/openapi-generator#2834)

valb3r · 2020-04-27T17:26:40Z

fintech-examples/fintech-ui/src/app/common/navbar/navbar.component.ts

@@ -29,7 +30,7 @@ export class NavbarComponent implements OnInit {
  getSessionValidUntil(): string {
    const validUntilDate: Date = this.storageService.getValidUntilDate();
    if (validUntilDate !== null) {
-      const validUntilDateString = validUntilDate.toLocaleString();
+      const validUntilDateString = toLocaleString(validUntilDate);
      const regEx = /.*([0-9]{2}:[0-9]{2}:[0-9]{2})/;


Why this regex is needed?

var time = date.toLocaleTimeString('en-GB')

will return time in en-GB locale

valb3r · 2020-04-27T17:44:00Z

fintech-examples/fintech-ui/src/app/services/storage.service.ts

-    const date =  new Date(parseInt(validUntilTimestamp, 10));
-    if (date.toLocaleString() === 'Invalid Date') {
+    const date = new Date(parseInt(validUntilTimestamp, 0));
+    if (toLocaleString(date) === 'Invalid Date') {


Using this hardcoded literal is really bad. I would advise using date library like moment or luxon for date operations - and I see there are lots of date operations

valb3r · 2020-04-27T17:50:20Z

fintech-examples/fintech-ui/src/app/login/login.component.ts

@@ -16,7 +16,10 @@ export class LoginComponent implements OnInit {
  ngOnInit() {
    this.loginForm = this.formBuilder.group({
      username: ['', [Validators.required, Validators.pattern('^[a-z0-9]+$')]],
-      password: ['', [Validators.required, Validators.pattern('.{4}')]]
+      password: ['', Validators.required]


Since backend requires 4 character password to register new user - this one was used

valb3r · 2020-04-27T17:52:39Z

.../fintech-impl/src/main/java/de/adorsys/opba/fintech/impl/service/RedirectHandlerService.java

        String xsrfToken = UUID.randomUUID().toString();
        HttpHeaders authHeaders = authorizeService.modifySessionEntityAndCreateNewAuthHeader(restRequestContext.getRequestId(), sessionEntity,
                xsrfToken, cookieConfigProperties, SessionCookieType.REGULAR);
-        authHeaders.put(LOCATION_HEADER, singletonList(redirectUrls.getOkStatePath()));
+        authHeaders.put(LOCATION_HEADER, singletonList(redirectUrl));


Why not simply authHeaders.set instead of put with list?

OBG-442 Date formatting problem solved

8905f00

electronicpeter requested review from rjtch and vlad-nechai April 22, 2020 12:53

valb3r reviewed Apr 22, 2020

View reviewed changes

obg 442 with npm run ng lint

c47e387

valb3r reviewed Apr 23, 2020

View reviewed changes

Hergy Tchuinkou and others added 6 commits April 24, 2020 10:37

resolved conflicts

f4829a3

OBG 442 timeformatting

ff38e4c

path correction

8c1fb57

OBG 442 continued redirect is working including cancel buttons

0c31d2c

obg 442 fix ui test

67635d6

OBG 442 finally tested

187250e

electronicpeter changed the title ~~OBG-442 Date formatting problem solved~~ OBG-442 Date formatting problem solved and deny consent redirects done Apr 27, 2020

elroman reviewed Apr 27, 2020

View reviewed changes

OBG 442 ok/notok as enum

83bfd62

valb3r reviewed Apr 27, 2020

View reviewed changes

valb3r approved these changes Apr 27, 2020

View reviewed changes

peter and others added 2 commits April 27, 2020 23:40

442 force rebuild

0568b0d

Merge branch 'develop' into 442-continued

d2282b8

electronicpeter merged commit 945812c into develop Apr 27, 2020

electronicpeter deleted the 442-continued branch April 27, 2020 23:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OBG-442 Date formatting problem solved and deny consent redirects done #605

OBG-442 Date formatting problem solved and deny consent redirects done #605

electronicpeter commented Apr 22, 2020

valb3r Apr 22, 2020

electronicpeter Apr 23, 2020

electronicpeter Apr 23, 2020

valb3r Apr 22, 2020

electronicpeter Apr 23, 2020 •

edited

Loading

valb3r Apr 23, 2020

electronicpeter Apr 27, 2020 •

edited

Loading

codecov-io commented Apr 23, 2020 •

edited

Loading

valb3r Apr 23, 2020

electronicpeter Apr 27, 2020

valb3r Apr 23, 2020

elroman Apr 27, 2020

electronicpeter Apr 27, 2020

valb3r Apr 27, 2020

valb3r Apr 27, 2020

valb3r Apr 27, 2020

valb3r Apr 27, 2020

OBG-442 Date formatting problem solved and deny consent redirects done #605

OBG-442 Date formatting problem solved and deny consent redirects done #605

Conversation

electronicpeter commented Apr 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

electronicpeter Apr 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

electronicpeter Apr 27, 2020 • edited Loading

Choose a reason for hiding this comment

codecov-io commented Apr 23, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

electronicpeter Apr 23, 2020 •

edited

Loading

electronicpeter Apr 27, 2020 •

edited

Loading

codecov-io commented Apr 23, 2020 •

edited

Loading