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

Jump to bottom

Autocomplete fixes 2 #992

Draft
wants to merge 5 commits into
base: dev
Choose a base branch
from
Draft

Autocomplete fixes 2 #992

wants to merge 5 commits into from

Conversation

rwakulszowa
Copy link
Member

@rwakulszowa rwakulszowa commented Jun 17, 2021 •
edited
Loading

Fixes #948

Sortowanie odpowiedzi autocomplete na podstawie podanego parametru case.

TODO: Wymagana zmiana na froncie (podanie argumentu case przy zapytaniu)

Dodana funkcjonalność:

  • każde pole Select na start odpytuje autocomplete API o początkowy zestaw sugestii. Po kliknięciu w pole użytkownik zawsze otrzyma jakieś sugestie (o ile odpowiednie dane są w bazie, rzecz jasna), bez potrzeby wpisywania niczego w pole tekstowe. Po wpisaniu tekstu użytkownik otrzyma sugestie na podstawie tekstu, tak jak do tej pory.
  • dla niektórych typów pół (na ten moment - tylko Instytucja), klient poda dodatkowo powiązane id sprawy. Sugestie autocomplete będą preferowały obiekty powiązane z daną sprawą, tj. jeśli pasujących sugestii jest 100, a klient wyświetla ich max. 20, to system zwróci w pierwszej kolejności obiekty przypisane do danej sprawy.
    Ważne: dla wygody użytkownika, sugestie są sortowane alfabetycznie przed wyświetleniem, tj. system postara się, żeby lista sugestii zawierała powiązane obiekty (instytucje), ale nie zagwarantuje, że będą one na samej górze widocznej listy - końcowa lista zawiera dane w alfabetycznej kolejności.
auto.mp4

@ad-m :

  • czy konieczne będzie tutaj wygenerowanie SDK, żeby użyć nowego parametru?
  • czym są "sygnatury" wspomniane w automatyczne podpowiadanie  #948?
  • czy opisane wyżej podejście spełnia wymagania?
  • nowa funkcjonalność teoretycznie otwiera możliwość wykrycia które sprawy są powiązane z którymi instytucjami (tj. użytkownik może odpytywać API autocomplete z nowym polem case i na podstawie zwracanych wyników dowiedzieć się która sprawa jest powiązana z daną instytucją; nie sprawdzamy tutaj żadnych uprawnień - czy to problem, czy (póki co?) zakładamy, że dostęp do odczytu tego typu danych jest raczej powszechny / nie mniej powszechny niż dostęp do autocomplete API?)

@rwakulszowa rwakulszowa force-pushed the autocomplete-fixes-2 branch from 3d3ea09 to 37927f9 Compare June 17, 2021 14:12
@ad-m
Copy link
Member

ad-m commented Jun 17, 2021

(pending #991)

Fixes #948

Sortowanie odpowiedzi autocomplete na podstawie podanego parametru case.

TODO: Wymagana zmiana na froncie (podanie argumentu case przy zapytaniu)

Dodana funkcjonalność:

* każde pole `Select` na start odpytuje autocomplete API o początkowy zestaw sugestii. Po kliknięciu w pole użytkownik zawsze otrzyma jakieś sugestie (o ile odpowiednie dane są w bazie, rzecz jasna), bez potrzeby wpisywania niczego w pole tekstowe. Po wpisaniu tekstu użytkownik otrzyma sugestie na podstawie tekstu, tak jak do tej pory.

Super.

* dla niektórych typów pół (na ten moment - tylko Instytucja), klient poda dodatkowo powiązane id sprawy. Sugestie autocomplete będą preferowały obiekty powiązane z daną sprawą, tj. jeśli pasujących sugestii jest 100, a klient wyświetla ich max. 20, to system zwróci w pierwszej kolejności obiekty przypisane do danej sprawy.

Przez klienta masz na myśli klienta API (nie użytkownika)?

  Ważne: dla wygody użytkownika, sugestie są sortowane alfabetycznie przed wyświetleniem, tj. system postara się, żeby lista sugestii zawierała powiązane obiekty (instytucje), ale _nie_ zagwarantuje, że będą one na samej górze widocznej listy - końcowa lista zawiera dane w alfabetycznej kolejności.

Myślę, że to będziemy mogli zweryfikować po chwili użytkowania przez Agnieszkę.

@ad-m :

* czy konieczne będzie tutaj wygenerowanie SDK, żeby użyć nowego parametru?

SDK powinno się samo wygenerować i wejdzie PR z aktualizacją SDK. Bez nowego SDK parametr nie będzie przekazywany.

* czym są "sygnatury" wspomniane w #948?

Sygnatury to identyfikatory listu / sprawy, które urzędy / my nadajemy, aby identyfikować list. Przykładowy zrzut ekranu:

obraz

Tu więcej sygnatur: https://orzeczenia.nsa.gov.pl/cbo/search

Podpowiadanie sygnatur jest inne niż w pozostałych przypadkach, bo sygnatura nie jest obiektem, a wartością we właściwości istniejącego obiektu.

* czy opisane wyżej podejście spełnia wymagania?

W mojej ocenie takie podejście więcej niż spełnia wymagania.

* nowa funkcjonalność teoretycznie otwiera możliwość wykrycia które sprawy są powiązane z którymi instytucjami (tj. użytkownik może odpytywać API autocomplete z nowym polem `case` i na podstawie zwracanych wyników dowiedzieć się która sprawa jest powiązana z daną instytucją; nie sprawdzamy tutaj żadnych uprawnień - czy to problem, czy (póki co?) zakładamy, że dostęp do odczytu tego typu danych jest raczej powszechny / nie mniej powszechny niż dostęp do autocomplete API?)

Obecnie nie planujemy żadnego rozgraniczania praw do odczytu informacji, a na pewno nie na poziomie poszczególnych obiektów, więc jest to wystarczające. Póżniej będzie można wprowadzić kontrolę dostępu na polu "case".

Na szybko odniesienie się do pytań, nie przeglądałem całego kodu, bo urlopuje się.

@rwakulszowa
Copy link
Member Author

Przez klienta masz na myśli klienta API (nie użytkownika)?

Tak, mam na myśli przeglądarkę, bez dodatkowej akcji ze strony użytkownika.

Podpowiadanie sygnatur jest inne niż w pozostałych przypadkach, bo sygnatura nie jest obiektem, a wartością we właściwości istniejącego obiektu.

Jasne, postaram się to dodać w najbliższym czasie.

@rwakulszowa rwakulszowa force-pushed the autocomplete-fixes-2 branch from 9c3a40c to 609a3ce Compare July 14, 2021 08:12
@rwakulszowa
Copy link
Member Author

rwakulszowa commented Jul 14, 2021 •
edited
Loading

@ad-m - przy pracy nad sygnaturami zastanawiałem się czy warto wynieść pole Letter.reference_number to odrębnego modelu. Wydaje mi się, że wtedy łatwiej będzie dodać logikę autocomplete, ponieważ w dużej mierze jest opisana z myślą o modelach właśnie.

Pytanie brzmi - czy ta zmiana zaburzy jakoś strukturę danych (np. w stosunku do EOD v1), czy nie ma tutaj żadnego problemu?
Dodałem przykładową implementację jak wyglądałyby modele i migracja po przeniesieniu sygnatury / reference_number do oddzielnego modelu.

Wydaje mi się, że da się dodać autocomplete bez tego dodatkowego modelu, ale będzie to wymagało odrobinę więcej kodu.

@rwakulszowa rwakulszowa force-pushed the autocomplete-fixes-2 branch from 2bfcf05 to 88c2cd4 Compare July 15, 2021 07:44
@rwakulszowa rwakulszowa force-pushed the autocomplete-fixes-2 branch from 684e73a to f3fe403 Compare July 23, 2021 08:43
@rwakulszowa rwakulszowa force-pushed the autocomplete-fixes-2 branch from 80d8a39 to febe6cd Compare July 24, 2021 08:34
@ad-m
Copy link
Member

ad-m commented Jul 24, 2021

Zapoznałem się z podejściem do ReferenceNumber. Podoba mi się, bo dostrzegam jak uprości autocomplete. Najistotniejsze, że nie zmieniło się API do wprowadzenia treści. Słusznie dostrzegasz, że wymaga to zaktualizowania migratora, co – mam nadzieje – wyłapało CI.

Zastanawiam się czy nie warto tego rozdzielić na osobne PR, bo to szczególny, rozbudowany przypadek.

@rwakulszowa
Copy link
Member Author

Tak, CI wykryło brakujące zmiany przy migracji V1 -> V2.

Postaram się wkrótce wydzielić zmianę modelu do oddzielnego PR.

This was referenced Jul 25, 2021
Closed
Merged
@rwakulszowa
Fetch initial autocomplete suggestions.
ae854ac 
Before the user types something into the autocomplete search field,
fetch an initial set of suggestions.

Client changes only. Relevant backend changes (picking the right set
of initial suggestions) to be added in a separate commit.
@rwakulszowa
Prioritize related institutions in autocomplete.
fa36827 
The autocomplete endpoint accepts an optional `case` parameter.
The backend will put institutions related to `case` on top of the
suggestions list.
@rwakulszowa
Silence a type checker error
2cffe82
@rwakulszowa rwakulszowa force-pushed the autocomplete-fixes-2 branch from febe6cd to 2cffe82 Compare July 27, 2021 09:46
ad-m
ad-m reviewed Jul 27, 2021
View reviewed changes
frontend-project/src/components/FetchSelect.tsx
@@ -99,7 +99,10 @@ export function FetchSelect<
setRelatedItems([]);
setFetchingRelatedItems(true);

fetchRelatedItems(arrayValue)
// NOTE(rwakulszowa): `ValueType` is a bit complicated - converting it to
Copy link
Member

Choose a reason for hiding this comment

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

@kuskoman could you provide comment there?

@rwakulszowa rwakulszowa force-pushed the autocomplete-fixes-2 branch from c8fb80f to 264e73c Compare July 27, 2021 15:40
@rwakulszowa rwakulszowa force-pushed the autocomplete-fixes-2 branch from d6fc55e to 832d6c0 Compare July 27, 2021 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ad-m ad-m ad-m left review comments

@magul magul Awaiting requested review from magul magul will be requested when the pull request is marked ready for review magul is a code owner

Assignees
No one assigned
Labels
back-end front-end
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

automatyczne podpowiadanie
2 participants
@rwakulszowa @ad-m