Trait-db v0 #722

Blodir · 2025-01-28T08:13:40Z

This branch includes changes to three projects (my bad):

Laji-api-client-b (small) (reviewer: Olli)
Laji-ui stuff (reviewer: Meeri)
- note: lu-form-string-list has no styling yet, but the code can be reviewed
Trait-db (reviewer: Robert)
- there are no translation keys yet
- trait-search-filters are missing many filters and datatable columns (I'm looking into generating these from typescript AST)
- trait-db-dataset html doesnt need to be checked, the code should be
- TraitDbNewTraitComponent doesn't need to be checked

Each could be reviewed separately by different people

TODO

aorin

Looked through the laji-ui project, looks good, left some small improvement suggestions

projects/laji-ui/src/lib/mixins/_fieldset.scss

aorin · 2025-01-30T10:33:22Z

projects/laji-ui/src/lib/forms/string-list/string-list.component.html

@@ -0,0 +1,5 @@
+<input type="text" (blur)="onTouched(value)" #textInput>
+<ul>


Looks like this is about the same as the pill-list (in shared-modules). Having almost the same functionality in many places makes the project more complex and harder to keep the styles uniform. Maybe not in this PR but in the future it would be good to check if it's easy to refactor it so that all the places that use this functionality would use the same component.

string-list implements ControlValueAccessor, while pill-list is literally just a list (non-semantic html one at that) where you can remove items. So they are different. In the future I'd like to make an enum-list that would replace pill-list in many (all?) places e.g. taxon search in observation form. The 'pills/tags' sort of UI also works better for enums than for arbitrary strings.

Btw, I think string-list could probably use a rename, maybe string-list-input and enum-list-input or something? Or input-string-list and input-enum-list

Sounds good. I'd prefer the name string-list-input instead of enum-list-input since they are not always basic enums like the taxon search

I mean string-list-input and enum-list-input would be different components. string-list-input for arbitrary strings and enum-list-input for, well, enums.

projects/laji-ui/src/lib/datatable/datatable.component.ts

aorin · 2025-01-30T11:09:40Z

projects/laji-ui/src/lib/datatable/datatable.component.ts

+import { ChangeDetectionStrategy, ChangeDetectorRef, Component, ElementRef, EventEmitter, Inject, Input,
+  OnChanges, OnInit, Output, QueryList, Renderer2, RendererStyleFlags2, SimpleChanges, TemplateRef, ViewChild, ViewChildren } from '@angular/core';
+
+type Keyable = string | number | symbol;


Could consider splitting the code into multiple files at least in the future since it's quite big already and probably going to get bigger if this datatable is going to be used in all places. For example the models could go to their own file and sorting functions to their own and the pagination to it's own component.

I've extracted the paginator into its own component. However the rest of the stuff I have to disagree with, imo splitting files is unnecessary when it doesn't come naturally. Eg. the type defs are directly describing the component so there's no reason they should be namespaced somewhere else.

Btw, I'm hoping that the datatable won't get any (or at least much) bigger.

Yeah it's a matter of preference. The reason why I like having the types elsewhere is that it's easier to see the example usage comment or the inputs when they are at the top of the file and that's what I'm usually interested about

...i/src/app/+trait-db/trait-db-datasets/trait-db-new-dataset/trait-db-new-dataset.component.ts

rpulkka

Reviewed Trait-db, and found no issues. Great work!

olzraiti · 2025-01-30T10:19:43Z

projects/laji-api-client-b/src/laji-api-client-b.service.ts

+  fetch<
+    T extends keyof paths & string,
+    U extends keyof paths[T] & string,
+    Responses extends WithResponses<paths[T][U]>['responses']
+  >(
+    endpoint: T,
+    method: U,
+    params: Parameters<paths[T][U]>,
+    requestBody?: ExtractRequestBodyIfExists<paths[T][U]>,
+    cacheInvalidationMs = 86400000 // 1day in ms
+  ): Observable<ExtractContentIfExists<Responses[IntersectUnionTypes<keyof Responses, HttpSuccessCodes>]>> {


path would be more accurate term than endpoint. Endpoint would be method + baseUrl + path.

It takes some mental effort to read paths[T][U]. It could help if the generic names would be P and M or even Path and Method.

params could be left optional to simplify usage.

U allows now btw the parameters property of the paths... Not really a practical problem, but could be tightened.

I renamed the types and tightened the type bounds for the method type. It now accepts only keys that are not undefined or never, and excludes "parameters".

If we have params as optional, then you could theoretically give erroneous input if the openapi schema requires that params are present. It's quite annoying that you have to add the argument to each call even if the type of params is undefined, but wcyd 🤷

I tested how openapi-fetch works. In the image you can see that it works correct: I tested with GET /collections and GET /documents: the latter throws an error because it has a required parameter. We could maybe look at how it works.

But I'm ok with approving this, and we can continue improving the client as we go.

projects/laji-api-client-b/src/laji-api-client-b.service.ts

aorin

The changes looks good, thanks!

aorin · 2025-01-31T11:51:09Z

projects/laji-ui/src/lib/datatable/datatable.component.html

+            <ng-container
+              *ngTemplateOutlet="columns[colIdx]['cellTemplate'];
+              context: columns[colIdx].prop
+                ? { $implicit: row, prop: getNestedValue(row, columns[colIdx]['prop']) }


Prop is a little confusing name, since prop is also the property name (=columns[colIdx].prop)

olzraiti

I left one more change request. Otherwise looking good 👍

olzraiti · 2025-02-04T12:35:21Z

projects/laji-api-client-b/README.md

+}
+```
+
+Queries are cached by default based on the path and the query params. Whenever a new non-get request is sent to the same path, the cache is flushed automatically.


This is out of date. If it's hard to document, maybe put a link to the review discussion that explains it? Or just remove it?

Or idk... it's not entirely out of date. It says "based on the path", which is correct. I mean it doesn't mention that the path params affect it also. Maybe we can update the README after we develop the other changes (like method wrappers get() put() etc)

olzraiti · 2025-02-04T12:45:20Z

projects/laji-api-client-b/src/laji-api-client-b.service.ts

+  // resolve path parameters (eg. replace '/{personToken}' with params.path.personToken)
+  for (let i = 1; i < segments.length; i++) {
+    const segment = segments[i];
+    segments[i] = segment.replace(/\{([^}]+)\}/, (match, p1) => (params as any)['path'][p1] ?? '');


I'm a bit worried about this. An empty string path param would cause a wrong endpoint being called, which would be a disaster. Could we throw an error for undefined values and empty strings, instead of doing ?? ''?

I think undefined values is also good to check, because we have so many cases of using non-null assertion for instance variables.

In practice the ?? '' bit should never be reached though, because of the strict typing in params 🤔

The strict typing doesn't protect against an empty string. An empty string will cause a wrong endpoint being called (for example trying to call GET /a/:id but actually calls GET /a/, which equals GET /a)

And I think undefined is also still quite possible to reach here, as there are so many non-null assertions etc in the repo

Blodir and others added 30 commits November 24, 2023 15:40

laji-api-client prototype

ed3caab

Merge branch 'development' into api-client-prototype

1310fc0

init trait-db and api-client

968ebb5

fix infinite loading when logged out

b145543

trait-db about page

23058c3

trait-db new dataset

a8aa5a8

use OnPush to trait-db components

b2b8f62

traits index page

791f29e

trait-db single trait view

435c5c6

trait-db new trait and new dataset fields

298eb83

Merge branch 'development' into trait-db-186748322

2d9d841

trait-db edit dataset

1727de1

trait-db indicate deletion of datasets

7f249e4

Merge branch 'development' into trait-db-186748322

d4632a8

automatic cache flushing

a07e766

new laji-ui datatable component

b92d5ec

trait-db-browse use lu-datatable

b78d3bd

lu-datatable improvements

1b55d19

lu-datatable custom cell templates

ee2cc17

remove console.log

aea5345

sortable should default to true

9ce4692

sorting should support default columns with custom sortfn

4bb40c2

paginator active styles

fca56a6

improve scroll css and default ghost size

72a6600

separate trait search from the trait browser page

58b982a

update schema

dbe6753

cache in-progress observables

0a97ef0

make datatable y-scrollable

6e91048

update swagger

ba52d01

trait-db trait-search-filters

4ae3ad0

Blodir marked this pull request as ready for review January 30, 2025 08:07

aorin self-assigned this Jan 30, 2025

rpulkka self-requested a review January 30, 2025 08:56

rpulkka self-assigned this Jan 30, 2025

rpulkka requested review from aorin and olzraiti January 30, 2025 08:58

fix laji-api-client base url

b91502d

aorin requested changes Jan 30, 2025

View reviewed changes

rpulkka reviewed Jan 30, 2025

View reviewed changes

...i/src/app/+trait-db/trait-db-datasets/trait-db-new-dataset/trait-db-new-dataset.component.ts Outdated Show resolved Hide resolved

rpulkka approved these changes Jan 30, 2025

View reviewed changes

olzraiti requested changes Jan 30, 2025

View reviewed changes

Blodir added 5 commits January 31, 2025 10:55

remove redundant style

89b7185

allow column templates to use prop

ba83865

check that defaultColumns isnt null

bd0990b

extract datatable paginator into its own component

0d57a41

rename new-dataset to dataset-editor

75c6c1b

Blodir requested a review from aorin January 31, 2025 10:18

tighten parameter type bounds

2d29dc3

aorin approved these changes Jan 31, 2025

View reviewed changes

Blodir added 4 commits February 2, 2025 20:13

provide baseurl as an injection token

cdb255a

update hash

a487661

variable level path caching

0070807

split path vars separately from proceeding path

0226bd8

Blodir requested a review from olzraiti February 3, 2025 16:21

Blodir added this to the Release 79 milestone Feb 4, 2025

olzraiti requested changes Feb 4, 2025

View reviewed changes

Blodir mentioned this pull request Feb 4, 2025

laji-api-client improvements #733

Open

2 tasks

remove trait-db link from beta

ef2496e

Blodir merged commit 575bae1 into development Feb 4, 2025
1 check passed

Blodir deleted the trait-db-186748322 branch February 4, 2025 15:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Trait-db v0 #722

Trait-db v0 #722

Blodir commented Jan 28, 2025 •

edited by rpulkka

Loading

aorin left a comment

aorin Jan 30, 2025

Blodir Jan 31, 2025

aorin Jan 31, 2025

Blodir Jan 31, 2025 •

edited

Loading

aorin Jan 30, 2025

Blodir Jan 31, 2025

aorin Jan 31, 2025

rpulkka left a comment

olzraiti Jan 30, 2025

Blodir Jan 31, 2025

Blodir Jan 31, 2025

olzraiti Feb 4, 2025 •

edited

Loading

aorin left a comment

aorin Jan 31, 2025

olzraiti left a comment

olzraiti Feb 4, 2025

olzraiti Feb 4, 2025

olzraiti Feb 4, 2025

Blodir Feb 4, 2025

olzraiti Feb 5, 2025

		@@ -0,0 +1,5 @@
		<input type="text" (blur)="onTouched(value)" #textInput>
		<ul>

Trait-db v0 #722

Trait-db v0 #722

Conversation

Blodir commented Jan 28, 2025 • edited by rpulkka Loading

aorin 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

Blodir Jan 31, 2025 • 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

rpulkka 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

olzraiti Feb 4, 2025 • edited Loading

Choose a reason for hiding this comment

aorin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olzraiti 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

Choose a reason for hiding this comment

Blodir commented Jan 28, 2025 •

edited by rpulkka

Loading

Blodir Jan 31, 2025 •

edited

Loading

olzraiti Feb 4, 2025 •

edited

Loading