NEWS-32 Dodanie logowania i rejestracji firebasowej #28

kightsonsanom · 2020-11-21T12:10:36Z

kightsonsanom · 2020-11-21T12:12:05Z

lib/src/di/injector.dart

@@ -53,20 +55,25 @@ void injectDependencies(Environment flavor) {
  _locator.registerSingleton<AppConfig>(AppConfig());
  _locator.registerSingleton<AppInfoRepository>(AppInfoLocalRepository());
  _locator.registerSingleton<PersistenceRepository>(SharedPreferencesPersistenceRepository());
-  _locator.registerSingleton<SettingsRepository>(SettingsLocalRepository(_locator<PersistenceRepository>()));
-  _locator.registerSingleton<PresentationShowingRepository>(PresentationShowingLocalRepository(_locator<PersistenceRepository>()));
+  _locator.registerSingleton<SettingsRepository>(


większość zmian w tym pliku wynika z ctrl + alt + L, niestety z racji, że w robo mamy inny threshhold zawijania linii niż tu to ciężko mi to uspójnić 😞

nie boli :) możesz powiedzieć co tam masz poustawiane to ustawię tak samo, żeby z czasem każdy pull request nie zawierał więcej zmian formatowania niż tych tych faktycznych :D

w takim razie możesz ustawić tak

kightsonsanom · 2020-11-21T12:13:09Z

lib/src/ui/pages/home/home_page.dart

+                if (selectedPage == 0) {
+                  return Future.value(true);
+                } else {
+                  setState(() {


ten setState do wyjebania, bo notifier teraz zarządza stanem obecnej strony, żeby dało się cofać do tej samej strony po wejściu do settingsów

kightsonsanom · 2020-11-21T12:16:08Z

lib/src/ui/pages/settings/settings_page.dart

  @override
  _SettingsPageState createState() => _SettingsPageState();
 }

 class _SettingsPageState extends State<SettingsPage> {
+  final SettingsNotifier _settingsNotifier = inject<SettingsNotifier>();


zauważyłem, że łatwiej jest zarządzać stanem, kiedy każdy widok ma swojego notifiera inicjalizowanego w swojej klasie. Dzięki temu kiedy jakiś widok jest odpalany z kilku miejsc to nie trzeba się martwić o to czy jest notifier providowany przy nawigacji do takiego widoku + scope notifiera jest zawsze optymalny

marekvogt · 2020-11-22T14:17:39Z

lib/l10n/intl_en.arb

+  "register": "Register",
+  "registrationTitle": "Registration",
+  "name": "Name",
+  "email": "Email",


E-mail :)

marekvogt · 2020-11-22T14:22:37Z

lib/src/data/communication/firebase/firebase_authorization.dart

+import 'package:allthenews/src/domain/authorization/authorization.dart';
+import 'package:firebase_auth/firebase_auth.dart';
+
+class FirebaseAuthorization implements Authorization {


W zasadzie to Authentication :) - może FirebaseAuthenticationRepository i odpowiednio AuthenticationRepository interfejs? Trochę inaczej popakietować można, bo teraz w communication jest pakiet api i pakiet firebase a w zasadzie oba to api 🤔

proponuję to co w api obecnie dać do pakietu nytimes i wrzucić paczkę firebase do api

marekvogt · 2020-11-22T14:22:43Z

lib/src/data/communication/firebase/firebase_authorization.dart

+import 'package:firebase_auth/firebase_auth.dart';
+
+class FirebaseAuthorization implements Authorization {
+  FirebaseAuth auth = FirebaseAuth.instance;


marekvogt · 2020-11-22T14:23:02Z

lib/src/data/communication/firebase/firebase_authorization.dart

+  FirebaseAuth auth = FirebaseAuth.instance;
+
+  @override
+  Future<UserCredential> createUser(String email, String password) async =>


zbędne async, pozostałe metody to samo

marekvogt · 2020-11-22T14:25:31Z

lib/src/domain/authorization/authorization.dart

+
+abstract class Authorization {
+
+  Stream<User> userStream();


w takim klasycznym podejściu pole reprezentuje stan, a metoda zachowanie, więc jak userStream to fajnie jakby było polem, jak metoda, to getUserStream albo observeUserChanges :)

observeUserChanges mi się bardziej podoba :)

marekvogt · 2020-11-22T14:30:09Z

lib/src/main_common.dart

  injectDependencies(flavor);
-  FlutterError.onError = Crashlytics.instance.recordFlutterError;
+  WidgetsFlutterBinding.ensureInitialized();
+  await Firebase.initializeApp();


await w mainie blokuje wątek więc trochę słabo - i widzę, że tak jest w example na pub.dev, ale w dokumentacji już dużo ładniej to ograli:
https://firebase.flutter.dev/docs/overview#initializing-flutterfire
Generalnie jest issue, że na flutterze 1.20 nie śmiga, ale fixem jest pozostawienie WidgetsFlutterBinding.ensureInitialized() w mainie :)
Tu wspomniane issue: firebase/flutterfire#3490

miałem to na liście rzeczy do poprawienia, ale cocoapods na iosie się zjebał i potem już zapomniałem :p

marekvogt · 2020-11-22T14:35:18Z

lib/src/ui/common/mixin/error_mixin.dart

+import 'package:flutter/material.dart';
+
+mixin ErrorMessage {
+  Widget buildErrorMessage(FirebaseAuthException error) => error != null


jako, że jest w pakiecie common to powinien być bardziej generyczny tj. przyjmować Exception, chyba, że ma być typowo pod FirebaseAuthException, wówczas chyba powinien być gdzieś w pakiecie authentication i ewentualnie mógłby nazywać się AuthenticationErrorMessage i odpowiednio buildAuthenticationErrorMessage

to błąd, który będzie raczej używany tylko przy autentykacji, więc wybiorę tą drugą opcję

marekvogt · 2020-11-22T14:40:43Z

lib/src/ui/pages/profile/auth/auth_text_form_field.dart

+  });
+
+  @override
+  Widget build(BuildContext context) {


Zastanawiam się czy podejście jest poprawne, bo skoro ChangeNotifier służy do zarządzania stanem to wydaje mi się, ze powinno być tak, że kiedy np. wpisuje się coś do pola password, to to triggeruje metodę notifier.updatePassword(password), co zmienia wewnatrz notifiera stan (state.password), który potem z powrotem propagowany jest na UI do TextFormField i analogicznie z walidacją, tj. po wciśnięciu przycisku wykonywana jest walidacja w notifierze, który w przypadku pustych pól wrzuca do viewState błąd Pole nie może być puste, który dodawany jest do TextFormFielda

zauważ, że użyłem tu podejścia z widgetem Form - https://flutter.dev/docs/cookbook/forms/validation
to on przechowuje stan walidacji pod spodem i używa to tego klucza

final GlobalKey<FormState> _formKey = GlobalKey<FormState>();

notifier posiada tylko metodę to walidacji pól, ale postanowiłem lokalnie walidować tylko niepustość i dlatego każde pole ma taką samą metodę walidującą. Podejście o którym mówisz sprawdziłoby się gdyby przechowywanie stanu walidacji było całkowicie po naszej stronie

znaczy ja rozumiem to podejście, tylko chodzi mi o to, że jeżeli mamy architekturę, w której notifier propaguje stan na ui to powinien propagować wszystko, bo teraz część stanu jest zarządzana przez notifiera, a część przez sam ui 🤔 Dokumentacja flutterowa nie uwzględnia żadnej architektury. W necie za bardzo nie ma użycia providera z formularzem i jego walidacją (znalazłem jeden ale gość pomieszał bloc z providerem :P), ale chodzi mi o mniej więcej coś takiego jak w samplach flutter_bloc:
https://github.com/felangel/bloc/blob/master/examples/flutter_form_validation/lib/main.dart

w tym przykładzie co pokazałeś w ogóle nie jest używany widget Form, który ogarnia stany TextFormFieldów OOTB. Tak naprawdę jedyny powód, dla którego są tam TextFormFieldy to atrybut initialValue (który można by było też ograć na zwykłym TextFieldzie). Moim zdaniem jeżeli jest jakaś funkcjonalność dostępna przez framework to warto z niej skorzystać, jeżeli chcielibyśmy wszystko trzymać w stanie to równie dobrze pozycje scrolla moglibyśmy tam przechowywać, tylko po co?

Ja sugerowałem się przykładem z exampli do flutter_auth -> https://github.com/FirebaseExtended/flutterfire/blob/799231ac57a3005a4fbc8fd9c5951c3943e4a286/packages/firebase_auth/firebase_auth/example/lib/register_page.dart

W przypadku gdyby walidacja po stronie mobilnej byłaby bardziej skomplikowana i doszłoby do sytuacji gdzie trzeba customizować ten stan z Forma to wtedy opłacałoby się zachowywać ten stan ręcznie, ale w tym przypadku myślę, że skorzystanie z OOTB zachowania stanu ma sens

Ogólnie stan we flutterze można cały trzymać na ui i są wszystkie niezbędne narzędzia do tego, żeby zarządzać stanem tak jak to zostało zaprojektowane. Nie wymusza żadnej architektury. Jako, że nie wymusza architektury to powstało x różnych, które służą zarządzaniem stanu. Ten przykład z flutterfire nie ma żadnej architektury. Stan można ograć na UI, ale mimo wszystko stworzyłeś HomeNotifier (i słusznie), który wyciąga stan do notifiera, gdzie setState też to ogarnia, ale z notifierem jest zdecydowanie czyściej. Jeżeli przyjęta architektura wyciąga stan do notifierów i propaguje ten stan na ui to powinno być to konsekwetne. Jeżeli trzeba by było zarządzać samemu stanem scrolla to imo również powinien być propagowany z notifiera, ale na szczęście nie trzeba. Natomiast stan pól powinien być przechowywany w notifierze, ponieważ jest częścią stanu UI, który musi być zarządzany - tj. walidowany i wysyłany na backend. Sama walidacja też powinna odbywać się w walidatorze, który byłby gdzieś w domenie i notifier by sobie nim walidował pola przy kliknięciu buttona, a następnie wrzucił do viewState informację czy ma wyświetlić błedy walidacyjne i jak tak to które. W tym podejściu nie ma znaczenia czy walidacja jest skomplikowana czy nie, bo nawet jakby się skomplikowała to dla widgetów jest to niezauważalne, przeływ danych jest identyczny :) Kolejna kwestia to, że trzymanie wszystkiego w jednym miejscu (notifierze) pozwoli przetestować logikę klasę w izolacji od UI :)

trochę nie widzę tej granicy kiedy korzystać z narzędzi ogarniających stan OOTB, a kiedy już samemu tym zarządzać. W tym przypadku widgety Form i TextFormField stają się bezużyteczne. Jeżeli chcemy mieć stan w notifierze + korzystać z tego co oferuje framework to może trzymać w stanie notifiera sam GlobalKey ?

Jeżeli jedyną wartością Form i TextFormField jest zarządzanie stanem na ui to przy obecnej architekturze rzeczywiście są bezużyteczne, ale nie powiedziałbym, że to coś złego. Wiesz, w androidzie jak robisz MVP to nie pchasz LiveDaty na UI tylko dlatego, że jest coś takiego, tylko trzymasz się założeń. Różnica między tym co mamy a blocem jest taka, że tam wszystko opiera się na strumieniach i powiedzmy, że jest większe wyróżnienie istoty eventów propagowanych z ui do bloca. I wg sampli też nie korzysta z walidacji, którą daje Form i to też nic złego. Takie podejście fajnie się sprawdza na pewno przy Simple app state management (na zakładce List of state management approaches na flutter.dev), ale założenia, które przyjęliśmy przy providerze są jednak inne. Imo nie ma co mieszać dwóch podejść i ładnie zrobić stan samemu, bo wpychanie GlobalState, który rozszerza State<StatefulWidget>> to trochę jednak wpychanie UI wyżej (do notifiera) niż powinien być (UI)

ok, przekonałeś mnie, zrobię zarządzanie stanem walidacji z poziomu notifiera 😄

marekvogt · 2020-11-22T14:43:17Z

lib/src/ui/pages/profile/auth/authentication_page.dart

+          FlatButton(
+            onPressed: () => _navigateTo(LoginPage()),
+            child: Text(
+              Strings.of(context).login,


konsekwentnie moglibyśmy zawsze używać Strings.current :)

marekvogt · 2020-11-22T14:45:05Z

lib/src/ui/pages/profile/auth/authorization_notifier.dart

+  void _updateUserName(User user, String name) {
+    authorization
+        .updateUser(name)
+        .then((_) => returnToProfile.call())


returnToProfile?.call() i w pozostałych miejscach też z ?

no właśnie zastanawiałem się czy to konieczne, dałem z ? przy logoucie bo to jest na ekranie profilu i z tamtego miejsca nie przekazujemy callbacku do cofania. Pozostałe dwa miejsca przy metodach 'signIni_updateUserName` będą wywołane tylko po wejściu do erkanu logowania/rejestracji, więc ten callback nie powinien być tam nullem 🤔

a poza tym to myślałem czy warto na tym etapie przerzucać się na navigation 2.0, co myślisz?

powiedzmy, że z poziomu samego notifiera widzisz tylko, że callback może być zainicjalizowany ale nie musi, bo nie jest to realizowane przez konstruktor, więc w przypadku kiedy zawsze będzie ? nie ma potrzeby zastanawiania się czy ma prawo wystąpić czy nie, bo w zależności od użycia może ;) Co do navigation 2.0 to jak najbardziej :)

ma to sens, dodam ?. Dodałem też taska na trello do navigatora :)

marekvogt · 2020-11-22T14:46:17Z

lib/src/ui/pages/profile/auth/authorization_notifier.dart

+import 'package:flutter/widgets.dart';
+
+class AuthorizationNotifier extends ChangeNotifier {
+  final Authorization authorization = FirebaseAuthorization();


private + wstrzykiwanie do konstruktora :)

marekvogt · 2020-11-22T14:47:42Z

lib/src/ui/pages/profile/auth/authorization_notifier.dart

+
+  VoidCallback returnToProfile;
+
+  void getCurrentUser() {


dziwnie wygląda metoda wyglądająca jak getter, która nic nie zwraca ;)

marekvogt · 2020-11-22T14:53:05Z

lib/src/ui/pages/profile/profile_page.dart

+          final viewState =
+              providerContext.select((AuthorizationNotifier notifier) => notifier.state);
+
+          if (viewState.user == null) {


można by było zrobić jakoś bardziej generycznie bo jak domniemam, każdy ekran, który będziemy mieli w profilu będzie musiał sprawdzać czy użytkownik jest zalogowany? Więc może jakiś abstrakcyjny notifier i page dla tych ekranów? :)

co masz na myśli pisząc "każdy ekran w profilu"? zakładałem, że to będzie tylko jeden widok. Tak czy inaczej rzeczy związane z profilem zostawiłbym na taska z profilem

chodzi mi o to, że z poziomu profilu będziemy przechodzić na inne zakładki i one też będą autoryzowane, bo w teorii może się zdarzyć, że ktoś zostanie na tym ekranie, apkę zrzuci na stos, potem wróci i już nie będzie zalogowany? Może być osobno przy pierwszym ekranie profilowym, jak najbardziej :)

może się tak zdarzyć jakby usunąć usera ręcznie z firebase i chyba jakby się ktoś w międzyczasie na innym device zalogował, ale nie sprawdzałem tego w sumie

marekvogt · 2020-11-22T14:55:16Z

lib/src/ui/pages/profile/profile_page.dart

+                children: [
+                  Text(_authorizationNotifier.state.user?.email),
+                  Text(_authorizationNotifier.state.user?.displayName),
+                  Text("User uid: ${_authorizationNotifier.state.user?.uid}"),


do stringsów :)

docelowo myślę, że nie powinniśmy tej informacji wyświetlać na UI, dałem bardziej na potrzeby debugowania, proponuję, że dodam TODOsa na usunięcie tego przy okazji implementacji widoku profilu?

zawsze można ograć ładnie, tak żeby wyświetlać tylko w devowej wersji, tj. notifier miałby informację czy wyświetlać czy nie na podstawie Environment :) Chyba, że faktycznie do wywalenia, to może być todos :)

raczej mało przydatne info, mail jest i tak unikatowy, w sumie to już teraz wywalę

marekvogt · 2020-11-22T15:01:19Z

lib/src/domain/authorization/authorization.dart

+
+  Stream<User> userStream();
+
+  Future<UserCredential> createUser(String email, String password);


domena powinna być czysto dartowa, więc mimo wszystko zbudowałbym swoje obiekty domenowe zamiast korzystania z tych firebasowych, żeby ich zasięg kończył się na api, a nie przechodził przez domenę aż do ui

marekvogt · 2020-11-22T15:10:28Z

lib/src/ui/pages/profile/auth/authorization_notifier.dart

+import 'package:firebase_auth/firebase_auth.dart';
+import 'package:flutter/widgets.dart';
+
+class AuthorizationNotifier extends ChangeNotifier {


imo osobny notifier dla profilu, rejestracji i logowania, profil korzysta tylko z userStream, rejestracja z createUser, a logowanie z signIn

marekvogt · 2020-11-22T15:17:25Z

Przeszedłem na login i z jego poziomu wszedłem na ustawienia i zmieniłem theme (druga sprawa, że ustawienia imo mogłyby zniknąć z ekranu logowania/rejestracji)

marekvogt · 2020-11-22T15:20:16Z

Brakuje paddingu

I ogólnie ten font taki nytimesowy zostawiłbym na tylko w appbarach :)

marekvogt · 2020-11-22T15:21:19Z

A no i wszystkie komunikaty niezależnie od języka przychodzą po angielsku, więc musiałoby być u nas mapowanie błędów firebasowych na nasze komunikaty

marekvogt · 2020-11-22T15:28:05Z

jak jestem zalogowany to przy przechodzeniu z dashboardu na profil ekran miga - na początku jest ekran logowania potem dopiero profil, może dałoby radę ograć to jakoś loadingiem, bo jednak niefajnie wygląda to mignięcie

marekvogt · 2020-11-22T15:31:15Z

lib/src/ui/pages/profile/auth/auth_text_form_field.dart

+          border: OutlineInputBorder(
+            borderRadius: BorderRadius.circular(_Constants.borderRadius),
+          ),
+          hintStyle:


hint mógłby być innego koloru niż tekst

marekvogt · 2020-11-22T15:36:45Z

lib/src/ui/pages/profile/auth/auth_text_form_field.dart

+        controller: textController,
+        obscureText: obscureText,
+        decoration: InputDecoration(
+          labelText: hint,


jakoś dziwnie wygląda jak do labelText jest przypisywany hint, trudno mi to wytlumaczyć, ale jakby było odwrotnie to jakoś mniej dziwnie by mi to wyglądało, może dlatego, ze każdy label pasuje jako hint ale nie każdy hint pasuje jako label 🤔 - nie wiem czy słusznie, teraz przyszło mi do głowy :D

marekvogt · 2020-11-22T15:41:58Z

lib/src/ui/pages/profile/auth/authorization_notifier.dart

+    notifyListeners();
+  }
+
+  void _onUserAuthFailed(exception) {


brakuje typu dla exception

marekvogt · 2020-11-22T15:48:47Z

lib/src/ui/pages/profile/auth/login_page.dart

+                  child: Column(
+                    mainAxisAlignment: MainAxisAlignment.center,
+                    children: [
+                      AuthTextFormField(


pole email mogłoby mieć TextInputType.emailAddress :)

marekvogt · 2020-11-22T15:53:51Z

osobno wyciągnąłbym pakiety authentication i registration poza profile, bo jednak są to osobne domeny

marekvogt · 2020-11-22T15:58:37Z

lib/src/ui/pages/profile/auth/authentication_page.dart

+  }
+
+  Future _navigateTo(Widget page) {
+    return Navigator.pushReplacement(


ogólnie to ten return potrzebny? 🤔

marekvogt · 2020-12-09T18:47:14Z

lib/src/ui/pages/authentication/registration/registration_notifier.dart

+      _updateUserName(name);
+    } on Exception catch (exception) {
+      _setNotifierState(_state.copyWith(
+        exception: _exceptionMapper.toDomainException(exception),


mapowanie powinno odbyć się w warstwie data, gdzieś bliżej komunikacji, w tym przypadku w firebaseAuthenticationRepository

właśnie moim zdaniem powinny być mapowane jak najbliżej warstwy prezentacji, żeby nie przepychać tych błędów przez wszystkie warstwy, ale tak jak napisałem we wcześniejszym komentarzu

Co do exceptionów to myślę, że dobrze byłoby ten temat przegadać i uspójnić, celowo dałem to domeny, bo w ten sposób przy wielu warstwach tylko w jednym miejscu trzeba łapać wyjątek, a nie w każdej warstwie po kolei go przepychać,

Generalnie exceptiony są domeną. W warstwie prezentacji powinien być jakiś mapper, który na podstawie błędów domenowych wyświetla odpowiedni komunikat. Tak jak zwrotka w postaci sukcesu jest mapowana na obiekt domenowy np. Article, tak też wszystkie błędy powinny być mapowane w warstwie danych tak, aby warstwa domeny nie miała żadnych zależności komunikacyjnych, ani żeby błędy komunikacyjne nie przebijały się do warstwy prezentacji. Data korzysta z domeny, ui korzysta z domeny i to tyle z zależności między warstwami. Wiesz, skoro jsona mapujemy na domenę to czemu nie error? W przypadku przepychania błędów z api wprost do ui, domena i ui przechowują te błędy. I przykładowo dostaniemy DioError, czemu w ogóle UI ma wiedzieć, że korzystamy z Dio jako klienta http, albo z firebase? Ucinanie tego poprzez mapowanie w data powoduje brak konieczności wprowadzania zmian w pozostałych warstwach.

czyli wychodzi na to, że jedynym miejscem gdzie nie będzie mapowań jest właśnie domena. Tak to w data będzie mapowania błędów api/firebase czy tam cokolwiek innego na domenowe błędy, a w warstwie ui domenowe błędy na stringsy

Tak :) Analogicznie mapujemy teraz jeżeli chodzi o Article, w data z jsona na domenę, a w presentation z domeny na jakieś viewEntity w viewState :)

marekvogt · 2020-12-09T18:47:27Z

lib/src/ui/pages/authentication/registration/registration_notifier.dart

+import 'package:flutter/cupertino.dart';
+
+class RegistrationNotifier extends ChangeNotifier {
+  final AuthenticationRepository _authorizationRepository;


_authenticationRepository

marekvogt · 2020-12-09T18:50:43Z

lib/src/ui/pages/authentication/registration/registration_notifier.dart

+  VoidCallback returnToProfile;
+
+  Future<void> createUser(String email, String password, String name) async {
+    _validateFields();


Średnio mi się podoba bo ta walidacja częściowo jest w tej metodzie a częściowo w tym kolejnym ifie. Może jakby sygnatura wyglądała mniej więcej tak to zwięźlej by to wyglądało? 🤔 Taki luźny pomysł

_validateFields( onInvalid: (validationErrors) => ..., onValid: (state) => ... )

marekvogt · 2020-12-09T18:51:53Z

lib/src/ui/pages/authentication/registration/registration_notifier.dart

+    _setNotifierState(_state.copyWith(email: email));
+  }
+
+  void setName(String name) {


mega drobnostka ale może zamiast setName itd. to updateName (jakoś bardziej mi się kojarzy z aktualizacją stanu, ale może to moje jakieś takie spaczenie)

marekvogt · 2020-12-09T18:52:40Z

lib/src/ui/pages/authentication/login/login_state.dart

+
+class LoginState {
+  final bool isLoading;
+  final AllTheNewsException exception;


AuthenticationException :)

marekvogt · 2020-12-09T18:53:00Z

lib/src/ui/pages/home/home_page.dart

 class HomePage extends StatefulWidget {
+  final int initialPage;


w sumie HomePage nie musi trzymać initialPage, może to robić notifier co nie? 🤔

to w ogóle można wyjebać

marekvogt · 2020-12-09T18:53:15Z

lib/src/ui/pages/profile/profile_notifier.dart

+    _setNotifierState(ProfileState(exception: exception as AuthenticationException));
+  }
+
+  String checkIfEmpty(String text) => text.isEmpty ? Strings.current.emptyFieldError : null;


nieużywane

marekvogt · 2020-12-09T18:54:02Z

test/src/ui/pages/registration/registration_notifier_test.dart

+}
+
+extension on ChangeNotifier {
+  void verifyCreateUser(


jak nie będziesz niepotrzebnie przesyłał parametrów do metod create i signIn to nie będą potrzebne te extensiony :)

marekvogt · 2020-12-09T18:58:05Z

Co do tych za dużych tasków to racja, tutaj powiedzmy spokojnie można było wydzielić na szybko osobno logowanie, rejestrację i update firebase, ale powiedzmy, że nawet w obrębie jednej funkcjonalności (np. logowanie) można na trello do taska porobić checkboxy jako subtaski i wrzucać je osobno, po kolei, jak już ten, który weźmie taska sobie rozplanuje co i w jakiej kolejności chce zrobić :)

marekvogt · 2020-12-17T14:48:55Z

lib/src/domain/communication/firebase_exception.dart

@@ -0,0 +1,23 @@
+import 'package:allthenews/src/domain/communication/all_the_news_exception.dart';
+
+class AuthenticationApiException extends AllTheNewsException {}


powinno być w pakiecie lib/domain/authentication, bo domeną jest autentykacja

marekvogt · 2020-12-17T14:49:33Z

lib/src/data/communication/api/firebase/firebase_authentication_repository.dart

+import 'package:allthenews/src/domain/model/user.dart' as domain;
+import 'package:firebase_auth/firebase_auth.dart';
+
+class FirebaseAuthenticationRepository implements AuthenticationRepository {


zmieniłbym pakiet na lib/src/data/authentication

marekvogt · 2020-12-17T14:50:28Z

lib/src/domain/authorization/authentication_repository.dart

@@ -0,0 +1,13 @@
+import 'package:allthenews/src/domain/model/user.dart';
+
+abstract class AuthenticationRepository {


pakiet z lib/src/domain/authorization na lib/src/domain/authentication

do tej pory w authorization były rzeczy związane z kluczem api, jest to inny rodzaj autoryzacji niż logowanie od apki i dobrze by było to rozdzielić. Może obecne authorization zmienię na apiauthentication i zrobię osobny pakiet authentication na rzeczy związane z logowaniem/rejestracją?

Te rzeczy co były wcześniej w authorization niech zostaną w authorization, niech powstanie osobny pakiet authentication i do niego wrzuć te rzeczy z logowaniem, bo ten api key to autoryzacja, a logowanie to autentykacja

git, też o tym myślałem, ale zbyt podobne wydawały mi się te nazwy

marekvogt · 2020-12-17T14:51:15Z

lib/src/domain/communication/all_the_news_exception.dart

@@ -0,0 +1 @@
+abstract class AllTheNewsException implements Exception {}


dałbym w jakimś pakiecie lib/src/domain/common bo exceptiony w aplikacji niekoniecznie będą zawsze związane z jakąś komunikacją

marekvogt · 2020-12-17T14:58:15Z

lib/src/ui/common/message_provider.dart

@@ -0,0 +1,3 @@
+abstract class MessageProvider {
+  String getMessage(Object error);


Klasa mogłaby się nazywać ErrorMessageProvider skoro przyjmuje jako argument metody error

nie jest powiedziane, że tylko errory będziemy mapować na stringi, z takiego samego powodu AllTheNewsException dajemy do common. Za to nazwa parametru nie powinna się nazywać error, tylko jakiś identifier

w takim razie przyznam, że średnio mi się podoba przepychanie Object jako parametr, już lepiej jakby był generykiem:
MessageProvider<T> { String getMessage(T object) }

marekvogt · 2020-12-17T14:59:49Z

lib/src/domain/communication/firebase_exception.dart

@@ -0,0 +1,23 @@
+import 'package:allthenews/src/domain/communication/all_the_news_exception.dart';
+
+class AuthenticationApiException extends AllTheNewsException {}


lib/src/domain/authentication

marekvogt · 2020-12-17T15:12:45Z

lib/src/ui/pages/authentication/login/login_notifier.dart

+
+  void validateFieldsAndSignIn() {
+    _validateFields(
+        onInvalid: () {


onInvalid: () => notifyListeners()

marekvogt · 2020-12-17T15:13:08Z

lib/src/ui/pages/authentication/login/login_notifier.dart

+import 'package:flutter/cupertino.dart';
+
+class LoginNotifier extends ChangeNotifier {
+  final AuthenticationRepository _authorizationRepository;


_authenticationRepository

marekvogt · 2020-12-17T15:15:23Z

lib/src/ui/pages/authentication/field_error_message_provider.dart

+import 'package:allthenews/src/domain/authorization/authentication_field_error.dart';
+import 'package:allthenews/src/ui/common/message_provider.dart';
+
+class FieldErrorMessageProvider extends MessageProvider {


AuthFieldErrorMessageProvider

nie jest powiedziane, że tylko błędy autentykacji tu będą, obecnie tak jest, ale jak dojdzie walidacja innych pól to tu też będzie można je określić. Myślę, że nie warto tego rozdzielać, bo te błędy mogą się powielać (np pustość). Taki był zamysł i zmieniłbym nazwę AuthenticationFieldError na FieldError

marekvogt · 2020-12-17T15:21:24Z

lib/src/ui/pages/profile/profile_notifier.dart

+  void initUserState() {
+    _setNotifierState(_state.copyWithLoading(isLoading: true));
+    _authorizationRepository.observeUserChanges().listen((user) {
+      _setNotifierState(_state.copyWithUserAndLoading(user: user, isLoading: false));


w sumie może tu wystąpić błąd, i zastanawiam się czy stream może nic nie wysłać, wtedy też będzie wieczny loading

jeżeli nie będzie usera to zwróci nulla, każda inna zmiana będzie userem. Jeżeli chodzi o błędy to dokumentacja nic nie mówi na ten temat

a choćby brak neta?

hmm sprawdzę

jak nie ma neta to user leci jako null

ciekawe :D w takim razie jest git :D

w sumie słabo, bo jak rozpoznać czy jestem niezalogowany czy po prostu nie mam neta? dopiero przy logowaniu się dowiem

hmm... racja, zostawiłbym to na osobne zadanie, bo chyba musi być na tym ekranie jakiś dodatkowy check robiony 🤔

dodałem do trello

marekvogt · 2020-12-17T15:37:26Z

lib/src/ui/pages/profile/profile_page.dart

+        });
+  }
+
+  Widget _bindLoading() => const CircularProgressIndicator();


opakować w Center, żeby było na środku :)

marekvogt · 2020-12-17T15:41:52Z

test/src/ui/pages/login_notifier_test.dart

@@ -0,0 +1,53 @@
+import 'package:allthenews/src/domain/authorization/authentication_repository.dart';
+import 'package:allthenews/src/ui/common/message_provider.dart';
+import 'package:allthenews/src/ui/pages/authentication/authentication_message_provider.dart';


nieużywany import

marekvogt · 2020-12-18T14:05:42Z

wszystko spoko, tylko powód tego faila na bitrise, ale to jak już poprawisz to merguj :)

kightsonsanom requested a review from marekvogt November 21, 2020 12:10

kightsonsanom commented Nov 21, 2020

View reviewed changes

marekvogt reviewed Nov 22, 2020

View reviewed changes

marekvogt reviewed Dec 9, 2020

View reviewed changes

NEWS-32 Implementacja domenowych bledow autentykacji

a7edfae

kightsonsanom requested a review from marekvogt December 16, 2020 18:03

marekvogt reviewed Dec 17, 2020

View reviewed changes

NEWS-32 Pr comments, fix bug with ios build

17457a4

kightsonsanom requested a review from marekvogt December 18, 2020 13:45

marekvogt approved these changes Dec 18, 2020

View reviewed changes

kightsonsanom added 2 commits December 20, 2020 23:10

NEWS-32 Override firebase intl dependency

648611d

NEWS-32 Fix firebase crashlytics build error on ios

691cfc8

kightsonsanom merged commit e25f5d0 into master Dec 30, 2020


		Stream<User> userStream();

		Future<UserCredential> createUser(String email, String password);

		class HomePage extends StatefulWidget {
		final int initialPage;

		@@ -0,0 +1,23 @@
		import 'package:allthenews/src/domain/communication/all_the_news_exception.dart';

		class AuthenticationApiException extends AllTheNewsException {}

		@@ -0,0 +1,13 @@
		import 'package:allthenews/src/domain/model/user.dart';

		abstract class AuthenticationRepository {

		@@ -0,0 +1 @@
		abstract class AllTheNewsException implements Exception {}

		@@ -0,0 +1,3 @@
		abstract class MessageProvider {
		String getMessage(Object error);

NEWS-32 Dodanie logowania i rejestracji firebasowej #28

NEWS-32 Dodanie logowania i rejestracji firebasowej #28

Conversation

kightsonsanom commented Nov 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kightsonsanom Nov 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marekvogt Nov 22, 2020 • edited Loading

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

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

marekvogt Nov 23, 2020 • edited by kightsonsanom Loading

Choose a reason for hiding this comment

kightsonsanom Nov 23, 2020 • edited Loading

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

kightsonsanom Nov 22, 2020 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marekvogt Nov 23, 2020 • edited Loading

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

marekvogt commented Nov 22, 2020

marekvogt commented Nov 22, 2020

marekvogt commented Nov 22, 2020

marekvogt commented Nov 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

marekvogt commented Nov 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kightsonsanom Dec 9, 2020 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marekvogt commented Dec 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kightsonsanom Nov 21, 2020 •

edited

Loading

marekvogt Nov 22, 2020 •

edited

Loading

marekvogt Nov 23, 2020 •

edited by kightsonsanom

Loading

kightsonsanom Nov 23, 2020 •

edited

Loading

kightsonsanom Nov 22, 2020 •

edited

Loading

marekvogt Nov 23, 2020 •

edited

Loading

kightsonsanom Dec 9, 2020 •

edited

Loading