create new tag #658

daxter44 · 2020-10-13T07:59:22Z

#630 Wybrałem na początek Tag, bo nie ma zagnieżdżonych elementów.
Nie jest jeszcze obsłużony moment gdy dostanę odpowiedź 200 : Ok, na razie do momentu wysłania request'a do api.

Payload:

error:

vercel · 2020-10-13T07:59:27Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/watchdogpolska/small-eod/dtn6rpgpx
✅ Preview: https://small-eod-git-create-new-tag.watchdogpolska.vercel.app

ad-m · 2020-10-13T20:07:12Z

W przypadku uwierzytelniania z wykorzystaniem Basic Auth nie ma problemów:

W przypadku uwierzytelniania z wykorzystaniem cookies także nie ma problemów:

Taki stan występuje, gdy używam yarn start:public-api.

Jak uruchamiasz projekt? Możesz wykonać make build w /?

daxter44 · 2020-10-13T20:09:01Z

Uruchamiam przy pomocy docker-compose up, a jak powinienem ?

ad-m · 2020-10-13T20:15:05Z

Dobrze robisz. Ja poszedłem na skróty. Popracuje dalej nad odtworzeniem tego.

ad-m · 2020-10-13T20:29:12Z

Gdy uruchamiam w kontenerze wszystko mam w Chromium:

Jak również w Firefox:

Możesz skasować cookies i sprawdzić ponownie?

ad-m · 2020-10-13T20:44:26Z

Wygląda na to, że znalazłem winowajce tj. uwierzytelnianie sesją: https://www.django-rest-framework.org/api-guide/testing/#csrf
Docelowo powinniśmy je porzucić, więc proponuje przejść na uwierzytelnianie Basic Auth, które takich problemów nie wywołuje (mnie przegladarka sama pyta). Ewentualnie wykonanie #167, choćby w zakresie loginu i hasła. Chętnie pomogę.

daxter44 · 2020-10-13T20:48:27Z

@ad-m przerzuciłem się na chrome (wcześniej odpalałem z safari ) a tam na początku była ta sama dolegliwość.
Usunąłem pliki cookie, przeszedłem pod adres /tags/new i wyskoczyło mi okienko pop-up do zalogowania się. Uzupełniłem dane logowania i od tego momentu już żądanie przechodzi :)

ad-m · 2020-10-13T20:59:22Z

Świetnie! Daj znać, gdy pojawią się kolejne trudności. To, że miałem od Ciebie kod to byłoby znaczące przyspieszenie w analizie. Nie obawiaj się publikować kodu w toku, jeżeli się kompiluje (oczywiście, adekwatnie oznaczonego).

Jak wejdzie #613 to takie rzeczy dotyczące klienta API będę mógł wspierać szybciej, bo bezpośrednio z przeglądarki. Ewentualnie – w zakresie UX – same biuro powie czy poprawka spełnia potrzeby.

Docelowo powinniśmy wspierać zarówno Chrome i Firefox.

daxter44 · 2020-10-14T18:37:06Z

Po ostatnim commit, dodanie nowego tagu działa poprawnie. Z lewej strony ekranu wysuwa się powiadomienie że zapis zakończony sukcesem i przenosi użytkownika na listę z tagami.

Problem mam gdy chce dodać, tag który już istnieje.
Gdy status odpowiedzi jest inny niż 201, mój kod się nie wykonuje. Konkretniej chodzi mi o linię 36 z pliku /models/tags.ts
a na konsoli otrzymuje taki error:

Poza tym, metodę odpowiedzialną za powiadomienia dodałem do models/global.ts czy znacie inne miejsce gdzie ona by bardziej pasowała ? Tu zasugerowałem się nazwą pliku, jako globalny. Myślę że powinno się zrobić jeszcze enum z typami błędów jakie można przesyłać do metody z powiadomieniami.
Nie wiem za bardzo jak przenosić użytkownika na adres. Wzorując się na model/login.ts wykorzystuje router z paczki
umi. Wiem jednak, że od umi chcecie odchodzić dlatego myślę że jest to błędne rozwiązanie (mimo iż działające), jednak nie potrafię zrobić redirect w inny sposób.
Czekam na pomoc :)

ad-m · 2020-10-14T20:51:44Z

Łatwiej będzie wątkować dyskusje, jeżeli problemy będziemy. Tam jak napiszesz komentarz do kodu to mogę na niego odpowiedzieć i tworzą się subwątki, zamiast numerowania.

1/ Zobacz na https://github.com/watchdogpolska/small-eod-sdk-javascript/blob/f7a69184b467d3e1fc531328de48a650aeff9550/src/small_eod/TagsApi.js#L63-L67 i tu https://github.com/watchdogpolska/small-eod-sdk-javascript/blob/f7a69184b467d3e1fc531328de48a650aeff9550/src/ApiClient.js#L468-L494 . SDK zwraca Promise, która resolvuje się, w przypadku odpowiedzi 2xx, a throwuje gdy wystąpi błąd. Jeżeli używasz async...await to musisz używać try...catch, aby złapać taką promise, która throwuje. Zobacz na https://javascript.info/async-await#error-handling .

Przepisze to powiadomienia, aby to obsługiwać. Nie wiem czy koszernie z punktu Reacta, bo znam tylko JS.

2/ Nie mam pojęcia, nie znam Reacta, niestety.
3/ Myślę, że nie znajdziemy szybko rozwiązania, a to najwyżej będzie prosto wymienić. To jest raczej jako długofalowy efekt ze względu na koszt Umi, ale może jak poznamy bliżej to zaprzyjaźnimy się.

daxter44 · 2020-10-15T18:01:29Z

@ad-m Po Twoich zmianach, dodawanie nowego tagu wygląda już całkiem przyjemnie. Są przyjemne dla oka informacje o sukcesie oraz informacje o ewentualnym problemie.
Super by było, gdyby ktoś znający się na TS, React i redux rzucił na to okiem i wskazał co jest do poprawienia, ponieważ ja dopiero zaznajamiam się z tymi tematami.

Odnośnie powiadomień, utworzyłem demoniczny issue #666. Na tą chwilę nie potrafię utworzyć takiego elementu.

Gdybyś uznał, że wykonana w ramach tego PR praca jest w porządku i można to mergować to daj znać co zrobić dalej ?
(czy muszę utworzyć nowego PR który nie będzie draft, czy można jakoś zmienić typ tego PR )

ad-m · 2020-10-15T19:15:04Z

@daxter44 , status PR można zmieniać: https://docs.github.com/en/free-pro-team@latest/github/collaborating-with-issues-and-pull-requests/changing-the-stage-of-a-pull-request#marking-a-pull-request-as-ready-for-review Msz nawet nad polem komentarza przycisk "Ready for review"

ad-m · 2020-10-18T15:22:09Z

@mrbojko , w zagadnieniu przydatne byłoby osoby z doświadczeniem w React.

mrbojko · 2020-10-18T16:00:07Z

@mrbojko , w zagadnieniu przydatne byłoby osoby z doświadczeniem w React.

To może dodaj mojego brata do reviewersów - login fervero, może sprawdzić koszerność reactową...

ad-m · 2020-10-18T16:12:22Z

@mrbojko , w zagadnieniu przydatne byłoby osoby z doświadczeniem w React.

To może dodaj mojego brata do reviewersów - login fervero, może sprawdzić koszerność reactową...

Oznaczyć do review mogę tylko osoby dodane do repozytorium (co muszą zaakceptować) i osoby, które udzielały się w danym zagadnieniu.

kuskoman

to i przenieś z global do osobnego modelu tak jak mówiłem na callu

kuskoman · 2020-10-21T18:27:21Z

frontend-project/src/pages/tags/new/index.tsx


 interface TagNewFormProps {
-  name: String;
+  name: string;
  dispatch: Function;


Function to bardzo ogólny interfejs, pasowałoby coś bardziej konkretnego

Co proponujesz?

Redux (i react-redux) ma to już otypowane. Wydaje mi się, że można po prostu dać dispatch: Dispatch<AnyAction>.

@kuskoman a w zasadzie jak nazwać ten nowy model ?
Notification ?

Po przeniesieni metody do osobnego pliku rzuca takim błędem. plik notification.ts wygląda tak :

import { notification } from 'antd';
import { ReactNode } from 'react';
import { IconType } from 'antd/lib/notification';
export const openNotificationWithIcon = (message: IconType, description: ReactNode) => {
notification[message]({ message, description });
};

@daxter44 spushuj to zobaczę

@kuskoman 519fe3d ten commit

fervero · 2020-10-21T18:38:51Z

frontend-project/src/models/global.ts

 export interface GlobalModelState {
  collapsed: boolean;
 }

+export const openNotificationWithIcon = (type, msg) => {


Argumenty funkcji należy otypować

Zamiast pisać message: type, description: msg, można odpowiednio nazwać argumenty i zmieścić się w

export const openNotificationWithIcon = (message: IconType, description: ReactNode) => { notification[type]({ message, description }); }

Nie mogę znaleźć referencji do IconType, który sugerujesz. Nie widzę ani w React ani w Redux ani w Antd. Skąd ten twór ? Czy zostawić message: any

@daxter44 tak celem usprawiedliwienia- nie było mnie 2 dni w domu, teraz dopiero docieram do notyfikacji
myślę, że chodzi o:

import { IconType } from 'antd/lib/notification';

ad-m · 2020-10-21T18:45:52Z

frontend-project/src/models/tags.ts

+        router.replace(`/tags/`);
+      } catch (err) {
+        if (err.response.status === 400 && err.response.body.name) {
+          err.response.body.name.forEach(message =>


Czy możemy to zapisać generycznie, aby błędy w polach wyświetlały się przy polach formularzach, zamiast w powiadomieniu bez informacji jakiego pola dotyczy błąd?

Ja nie widzę problemu, aby dopracować strukturę odpowiedzi błędu API, ale potrzebne wytyczne co do potrzeb.

@ad-m W oknie logowania, mamy wyświetlanie problemu w formularzu (pomiedzy "Hasło logowania do konta" a polem z loginem ) I gdybym posiedział to jestem w stanie jeszcze to odwzorować.
Niestety nie wiem jak mogła by wyglądać struktura odpowiedzi z api aby przypisywać błąd do konkretnego pola w formularzu. Pozwolę sobie zawołać @kuskoman, czy jesteś w stanie coś tutaj zasugerować ?

Obecna odpowiedź API wskazuje konkretne pole, bo masz name. Ja proponuje starać się użyć obecnej odpowiedzi, a jak problem zostanie rozpracowany mocniej (więcej przypadków) to zmienimy strukturę API, aby uprościć implementacje w front-endzie. Wykonamy to przed wejściem z V2 na produkcje.

@ad-m siedziałem trochę godzin nad zagadnieniem, próbowałem szukać pomocy na forach jednak nie udało mi się zrobić odpowiedzi według Twoich założeń. Dokładniej mam problem z przekazaniem odpowiedzi z Modelu do formularza, próbowałem odgapić to z elementu logowania bądź z Cases/New (ponieważ są tam zwracane kolekcje tagów/instytucji itd.) jednak nic z tego.
Odniosę się tutaj do reszty issue, chętnie się za nie wezmę gdy ktoś opracuje standard edycji/dodawania/usuwania ponieważ tak jak mówiłem na pierwszym call'u to moje początki z reactem i reduxem i dopiero poznaje mechanikę ich działania :)

Ja w zakresie Reacta nie mam zbytniego doświadczenia także, ale podglądając zamykając projekt w Vue, z którym mam zaproponowałem:

const handleSubmit = (value: Tag) => { // dispatch({ // type: 'tags/create', // payload: { ...value }, // }); const response = { name: [`To pole jest wymagane ${Math.random()}.`] }; form.setFields(Object .entries(response) .map(([name, errors]) => ({ name, errors })) ); };

Teraz pozostaje tylko rozważenie gdzie ta logika obsługi błędów będzie się znajdować, czy w komponencie strony (jak teraz wkleiłem), czy w modelu (jak teraz jest), czy w serwisie (jak mogłoby teoretycznie być), a następnie odpowiednie przekazanie danych. Proszę o komentarz @fervero i @mateuszmagulski , którzy mają większe doświadczenie w projekcie.

Jeżeli będziemy mieli ten element poprawnie rozwiązany to możemy znacznie więcej walidacji pchnąć na back-end (na początku nawet nie musimy tracić czasu na komunikat dla wymaganego pola, bo back-end to sprawdzi). W zespole obecnie nie widzę osoby, która po prostu może wyznaczyć standard, więc raczej nie unikniemy tego.

@ad-m Ale taka walidacja na próbę zapisu pustego pola to była...

Ale mój kod nie odnosi się wyłącznie do wymaganego pola :) Kod może przetwarzać także pozostałe błędy z back-endu (nieprawidłowy format, złą długość itd.). Poza tym jest gwarantowane, że będzie to spójne z back-endem. Akurat tym razem przypadek nam się powtórzył, ale patrz szerzej jako na standard.

Tylko wąski zakres błędów jesteś w stanie sprawdzić po stronie front-endu.

Dowiozłem to ! Po wysłaniu tag'a treść błędu dotycząca pola name jest w: props.createdTag.tag?.name

vercel · 2020-10-25T09:04:40Z

@daxter44 is attempting to deploy a commit to the Watchdog Polska Team on Vercel.

A member of the Team first needs to authorize it.

vercel · 2020-10-26T01:53:42Z

Deployment failed with the following error:

The most recent charge for your active payment method has failed. Please update it here: https://vercel.com/teams/watchdogpolska/settings/billing

frontend-project/src/pages/tags/new/index.tsx

create new tag

0d1802a

boring-cyborg bot added the front-end label Oct 13, 2020

vercel bot deployed to Preview October 13, 2020 07:59 View deployment

ad-m linked an issue Oct 13, 2020 that may be closed by this pull request

dodanie Tagu #630

Closed

add notification after create tag

f21644b

vercel bot deployed to Preview October 14, 2020 18:28 View deployment

daxter44 mentioned this pull request Oct 14, 2020

dodanie Tagu #630

Closed

Catch SDK exception in tags create

721633d

vercel bot deployed to Preview October 14, 2020 21:16 View deployment

fix strings

9d6bb84

vercel bot deployed to Preview October 15, 2020 17:26 View deployment

add locales to notifications

41da224

vercel bot deployed to Preview October 15, 2020 17:49 View deployment

ad-m requested review from kuskoman and mateuszmagulski October 15, 2020 19:15

daxter44 marked this pull request as ready for review October 16, 2020 07:10

ad-m requested a review from mrbojko October 18, 2020 15:21

fix tags list path

a698cd8

kuskoman requested changes Oct 21, 2020

View reviewed changes

fervero reviewed Oct 21, 2020

View reviewed changes

ad-m reviewed Oct 21, 2020

View reviewed changes

daxter44 added 2 commits October 25, 2020 09:48

improvement of comments

519fe3d

improvement of comments

510359e

Add POC for field validation

6f7f7fe

show api response in form

93dd88d

ad-m reviewed Oct 26, 2020

View reviewed changes

frontend-project/src/pages/tags/new/index.tsx Outdated Show resolved Hide resolved

daxter44 added 3 commits October 27, 2020 07:16

fix errors displaying

fddfe84

change createTag property name to tagState

4a527dc

fix error map on submit

ecc6a69

vercel bot deployed to Preview November 4, 2020 19:12 View deployment

kuskoman approved these changes Dec 2, 2020

View reviewed changes

Merge branch 'dev' into create-new-tag

2442d64

vercel bot deployed to Preview December 16, 2020 19:41 View deployment

mrbojko approved these changes Dec 16, 2020

View reviewed changes

ad-m merged commit fd8e6a5 into watchdogpolska:dev Dec 17, 2020

create new tag #658

create new tag #658

Conversation

daxter44 commented Oct 13, 2020 • edited Loading

vercel bot commented Oct 13, 2020 • edited Loading

ad-m commented Oct 13, 2020

daxter44 commented Oct 13, 2020 • edited Loading

ad-m commented Oct 13, 2020

ad-m commented Oct 13, 2020

ad-m commented Oct 13, 2020

daxter44 commented Oct 13, 2020

ad-m commented Oct 13, 2020

daxter44 commented Oct 14, 2020

ad-m commented Oct 14, 2020

daxter44 commented Oct 15, 2020 • edited Loading

ad-m commented Oct 15, 2020

ad-m commented Oct 18, 2020

mrbojko commented Oct 18, 2020

ad-m commented Oct 18, 2020

kuskoman left a 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

daxter44 Oct 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fervero Oct 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kuskoman Oct 24, 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

ad-m Oct 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daxter44 Oct 26, 2020 • edited Loading

Choose a reason for hiding this comment

vercel bot commented Oct 25, 2020

vercel bot commented Oct 26, 2020

daxter44 commented Oct 13, 2020 •

edited

Loading

vercel bot commented Oct 13, 2020 •

edited

Loading

daxter44 commented Oct 13, 2020 •

edited

Loading

daxter44 commented Oct 15, 2020 •

edited

Loading

daxter44 Oct 25, 2020 •

edited

Loading

fervero Oct 21, 2020 •

edited

Loading

kuskoman Oct 24, 2020 •

edited

Loading

ad-m Oct 26, 2020 •

edited

Loading

daxter44 Oct 26, 2020 •

edited

Loading