Feature 683: Fixed server status check and made login async

[AdrianPlesner]

Closes #683
The check server status method did not request the correct api-endpoint, always resulting in 404 route not found. While fixing this, we also discovered that the login was completely backwards and not actually async. This, being related to the issue, has been fixed as well.

[codecov]

Codecov Report

Merging #686 (11beded) into develop (b0758a6) will decrease coverage by 0.2%.
The diff coverage is 28.5%.

@@            Coverage Diff            @@
##           develop    #686     +/-   ##
=========================================
- Coverage     86.3%   86.0%   -0.3%     
=========================================
  Files           76      76             
  Lines         3565    3584     +19     
=========================================
+ Hits          3078    3085      +7     
- Misses         487     499     +12     

Impacted Files	Coverage Δ
lib/blocs/auth_bloc.dart	32.6% <22.2%> (-13.2%)	⬇️
lib/screens/login_screen.dart	73.5% <36.3%> (+6.8%)	⬆️

[Determinatoro]

Please use the following checklist, @JeppeKLau. Feel free to check it off as you go along.

Assets

• The environments.json file has the following default values:

{
  "SERVER_HOST": "https://srv.giraf.cs.aau.dk/DEV/API",
  "DEBUG": true,
  "USERNAME": "Graatand",
  "PASSWORD": "password"
}

Code Design

• The code is in the right place? (Both in terms of folder structure and class structure)
• The code is not over-engineered. Examples of over-engineering: Implemented behavior for future problems
• This code could not, to the best of my knowledge, have reused existing code
• The code does not introduce functionality that is unnecessary for solving the problem

Code Readability

• Names are meaningful and self-documenting
• It is understandable, by reading the code, what it does
• The code is simple and readable. i.e. it contains no "hack", or obscure solution

Code Maintainability

• Has all the added code been properly commented

Only check one of the items below:

• The functionality does not need to be covered by tests
• The functionality needs to be covered by tests

Only check these if the functionality needs to be covered by tests:

• All functionality is covered by tests
• The tests has sensible names
• By reading the tests you know what they cover
• The tests cover sensible cases. Both happy paths and exception paths
• The tests cover the full functionality. i.e., The tests fails if some of the requested functionality is missing

Functionality

For this part, open the app, and test:

• The features that the code claims to implement, are actually implemented
• The code does not look like it has bugs
• All the new screens are reachable through in-app navigation

[Determinatoro]

Please use the following checklist, @Riasalit. Feel free to check it off as you go along.

Assets

• The environments.json file has the following default values:

{
  "SERVER_HOST": "https://srv.giraf.cs.aau.dk/DEV/API",
  "DEBUG": true,
  "USERNAME": "Graatand",
  "PASSWORD": "password"
}

Code Design

• The code is in the right place? (Both in terms of folder structure and class structure)
• The code is not over-engineered. Examples of over-engineering: Implemented behavior for future problems
• This code could not, to the best of my knowledge, have reused existing code
• The code does not introduce functionality that is unnecessary for solving the problem

Code Readability

• Names are meaningful and self-documenting
• It is understandable, by reading the code, what it does
• The code is simple and readable. i.e. it contains no "hack", or obscure solution

Code Maintainability

• Has all the added code been properly commented

Only check one of the items below:

• The functionality does not need to be covered by tests
• The functionality needs to be covered by tests

Only check these if the functionality needs to be covered by tests:

• All functionality is covered by tests
• The tests has sensible names
• By reading the tests you know what they cover
• The tests cover sensible cases. Both happy paths and exception paths
• The tests cover the full functionality. i.e., The tests fails if some of the requested functionality is missing

Functionality

For this part, open the app, and test:

• The features that the code claims to implement, are actually implemented
• The code does not look like it has bugs
• All the new screens are reachable through in-app navigation

[Riasalit]

Altså ved ikke om det her skyldes den her branch... men kan ikke logge ind

[AdrianPlesner]

@Riasalit Jeg har brug for at se mere af loggen fra dit api for at kunne identificere hvad der er galt, men mit bud er at det nok har noget at gøre med den kombination af web-api og api_client du bruger. Det er testet og virker med den nyeste version af develop af begge.

[AdrianPlesner]

Account controller I web-api er fornyligt ændret fra v1 til v2 så hvis man ikke får opdateret både web-api og api_client kan man ikke logge ind da end-points så ikke passer sammen.

[JeppeKLau]

Error handling er en stor mangel.

Udover kommentarene, kunne jeg forestille mig der var smartere måder at tjekke om vi har forbindelse til internettet, end blot at pinge Googles url. Ved godt det ikke er direkte relateret til issuet, men jeg vil nu stadig gerne se det ændret.

Umiddelbart vil jeg også sige at jeg ikke har hørt om den der Completer som bliver brugt. Kunne forestille mig at samme funktionalitet kan opnås ved bare at have nogle gode asynkrone funktioner, der bliver ventet på de rigtige steder.

Se også manglende punkter på checklisten.

[JeppeKLau]

Umiddelbart vil jeg mene at det ikke er smart bare at gennemtvinge en færdiggørelse ved en fejl.

[JeppeKLau]

Jeg kan forestille mig at man kan tilgå statusen på en anden måde, fx. gennem api klienten. Ellers burde det tilføjes.

Udover dette, giver navnet på variablen ikke mening da det ikke er på det endpoint vi logger ind.

[Riasalit]

Soooo flutter simply refuses to get the latest version of the api-client

[AdrianPlesner]

Soooo flutter simply refuses to get the latest version of the api-client

Yup det virker til at være problemet

[Riasalit]

We in bois.
"flutter packages upgrade" og så slet den forkerte cachede versio...... igen igen... virkede

[JeppeKLau]

Soooo flutter simply refuses to get the latest version of the api-client

Jeg fik det til at virke ved at fjerne api_client fra pubspec.yaml, køre flutter pub get, tilføje den tilbage i pubspec.yaml og køre flutter pub get igen - ser ud til at pubspec.lock har en forkert resolved-ref for api_client

[AdrianPlesner]

Soooo flutter simply refuses to get the latest version of the api-client

Jeg fik det til at virke ved at fjerne api_client fra pubspec.yaml, køre flutter pub get, tilføje den tilbage i pubspec.yaml og køre flutter pub get igen - ser ud til at pubspec.lock har en forkert resolved-ref for api_client

Jeg bruger samme methode

[Riasalit]

Soooo flutter simply refuses to get the latest version of the api-client

Jeg fik det til at virke ved at fjerne api_client fra pubspec.yaml, køre flutter pub get, tilføje den tilbage i pubspec.yaml og køre flutter pub get igen - ser ud til at pubspec.lock har en forkert resolved-ref for api_client

Jeg bruger samme methode

prøvede det. men det gjorde intet for mig

[Riasalit]

CI actions failer

[Riasalit]

Vil gerne have at der står hvad funktionen gør

[AdrianPlesner]

Vil gerne have at der står hvad funktionen gør

Ups. 😄

[JeppeKLau]

Se kommentarer :)

[JeppeKLau]

Jeg tænker at hvis man får en SocketException, så må der være noget galt med enten internet forbindelsen eller forbindelsen til serveren. Så jeg tænker det måske er fint nok bare at kontrollere forbindelsen til serveren, da det ikke er nødvendigt at tjekke forbindelsen til internettet hvis man alligevel får en SocketException.

[AdrianPlesner]

Det sørger jo bare for at håndtere fejl vi ikke regner med at der kommer. Det er bare for en sikkerheds skyld. Jeg tænker heller ikke at der kan være andet end manglede forbindelse til enten server eller internet, men man kan vel ikke være helt sikker. 🤷

[JeppeKLau]

Det er bare et spildt tjek. Desuden, som jeg også sagde før, er det virkelig ikke smart at tjekke om man har internet ved at pinge Google. Der er ikke noget der siger at deres service ikke kunne ske at gå ned på et tidpunkt i fremtiden.

[AdrianPlesner]

Det er da en meget standard måde at gøre det på, og så er det cross platform.

[JeppeKLau]

Der er det her plugin som jeg kan se ville kunne short circuit at tjekke om man har internet, inden man begynder på at pinge rundt på nettet.
Dertil vil jeg så nævne at der også findes det her plugin der automatisk pinger en række af hjemmesider og ikke blot en enkelt som vi gør her. Det giver en mindre chance for fejl da de alle skal misse før der konkluderes at man ingen forbindelse har.

De to plugins i sammenhæng kunne jeg se gav en god sikkerhed for at tjekke om man har internet eller ej.

[AdrianPlesner]

Oki doki så, hvis du insisterer.

[AdrianPlesner]

Vi kan ikke bruge connectivity pakken, den giver build errors på android, jeg håber du kan nøjes med den anden.

[JeppeKLau]

Det må være nok så - tænkte bare der måske var et eller andet indbygget, men det her må være nok

[JeppeKLau]

Når du har fjernet funktionen i koden, ser jeg ingen grund til at bringe funktionen tilbage til testen. Hvis testen er afhængig af funktionen, må testen skrives om så den ikke er afhængig af funktionen.

[AdrianPlesner]

Fixed

[JeppeKLau]

Vi er der næsten Adrian. Vil tilføje at det er dejligt at se du har rykket business logic om i BLOCen frem for at have det i screenen

[JeppeKLau]

Var det ikke den der skulle give en error tilbage når der skete en fejl?

[AdrianPlesner]

Joh, men det er egentligt nok at håndter fejler her. At smide fejler videre giver bare endnu et unødvendigt led med onError -> createErrorDialog, og i sidste ende giver det samme resultat

[JeppeKLau]

Jeg kan godt se ideen, men virker bare lidt fjollet når den kun bruges tre gange, hvor man bare kunne indsætte creatingNotifyDialog med den passende key

[AdrianPlesner]

Jah, jeg synes 3 gange var for meget