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

feat: add remoteDetectionAlert doc type #234

Merged
merged 12 commits into from
Sep 30, 2024
Merged

feat: add remoteDetectionAlert doc type #234

merged 12 commits into from
Sep 30, 2024

Conversation

tomasciccola
Copy link
Contributor

@tomasciccola tomasciccola commented Sep 2, 2024 •
edited
Loading

This adds the remoteDetectionAlert doc type. The geometry message is a stub for now, until @gmaclennan implements its decoding conversion to GeoJSON.

The only doubt I have is around sourceId. Is there a clear format for this string? Should we add concrete validation for it?

Also, I reused the tags type for metadata (since they are structurally the same?, basically a Record<string, any>). I know that semantically is confusing, but this allows to reuse all the conversion functions regarding tags

should close #228

@gmaclennan
Merge branch 'main' into feat/addRemoteDetectionAlert
1795cf4 
* main:
  chore: remove unused imports (#246)
  Release v3.0.0-next.26 (#242)
  fix: don't require iconRef for presets (#241)
  Release v3.0.0-next.25 (#240)
  chore!: make some Observation props optional (#239)
  chore!: make Field's `universal` property optional in schema (#236)
  test: use correct type for fixtures (#238)
  chore!: make Preset's `iconRef` optional (#235)
  feat: add `altitudeAccuracy` to observation metadata (#237)
@gmaclennan
WIP: 7b5de70 add failing test
c6ee774
@gmaclennan
fix: observation metadata types and encoding defaults
747289d
@gmaclennan
fix: don't generate partial and json methods (unused)
17b344e
@gmaclennan
Merge branch 'fix/observation-metadata' into feat/addRemoteDetectionA…
535e83c 
…lert

* fix/observation-metadata:
  fix: don't generate partial and json methods (unused)
  fix: observation metadata types and encoding defaults
  WIP: 7b5de70 add failing test
@gmaclennan
Add geometry property
7733276
@gmaclennan
Copy link
Member

This depends on #248 (because @comapeo/geometry doesn't export fromPartial methods, and it's easier to remove them from here than try to write then for Geometry), so that should be merged first.

The actual changes to add geometry are in https://github.com/digidem/mapeo-schema/pull/234/commits/7733276587d1d57109e502e635f914a7c9a896cc

It is a bit more complicated than I would like because of the need to deal with JSONSchema refs with 3 different libraries that we use (ajv, @json-schema-tools/dereferencer and @apidevtools/json-schema-ref-parser).

Would be nice to replace @json-schema-tools/dereferencer with @apidevtools/json-schema-ref-parser at some point, because the latter is already a dep of json-schema-to-typescript and we need to deal with it for that.

@EvanHahn
Copy link
Contributor

EvanHahn commented Sep 4, 2024

I plan to review this next week.

EvanHahn
EvanHahn requested changes Sep 8, 2024
View reviewed changes
Copy link
Contributor

@EvanHahn EvanHahn left a comment

Choose a reason for hiding this comment

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

I assume you and Gregor decided that this was a good format for alerts.

There's a bunch of Observation stuff in here that needs to be reverted. I think merging main should fix that.

Otherwise mostly LGTM.

proto/geometry/v1.proto Outdated Show resolved Hide resolved
proto/observation/v1.proto Outdated Show resolved Hide resolved
src/lib/decode-conversions.ts Outdated Show resolved Hide resolved
src/lib/decode-conversions.ts Show resolved Hide resolved
src/lib/decode-conversions.ts Outdated Show resolved Hide resolved
src/lib/encode-conversions.ts Outdated Show resolved Hide resolved
test/index.test.js Outdated Show resolved Hide resolved
schema/remoteDetectionAlert/v1.json
"required": [
"schemaName",
"detectionDateStart",
"detectionDateEnd",
Copy link
Contributor

Choose a reason for hiding this comment

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

Intuitively I don't know if this should be required. What if there's an alert that's still active?

Copy link
Member

Choose a reason for hiding this comment

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

This refers to the period during which the alert was detected in source imagery. As I understand it, in plain English, "We know that this issue was visible up to this date". It is the date of the last imagery available when the alert was detected. As such, I think it always exists when detecting alerts from remote sensing data.

The main advantage of making these required fields is a possible future feature where a user can filter the map view or a future alerts list view by date.

@gmaclennan
Merge branch 'main' into feat/addRemoteDetectionAlert
512d4e9 
* main:
  test: test validating a schema name that could "trick" JavaScript (#250)
  test: properly test more `validate` errors (#249)
  fix: observation metadata types (#248)
  chore: remove `any`s from decode helper function (#247)
@gmaclennan
rename geometry proto stub
11fbc82
gmaclennan
gmaclennan approved these changes Sep 9, 2024
View reviewed changes
Copy link
Member

@gmaclennan gmaclennan left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I think I've resolved all of Evan's comments and I've re-reviewed this, and it looks good to merge to me. @EvanHahn this needs a review approval from you to merge.

gmaclennan
gmaclennan requested changes Sep 9, 2024
View reviewed changes
Copy link
Member

@gmaclennan gmaclennan left a comment

Choose a reason for hiding this comment

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

I think we should not merge this until we have released the MVP, in case we need bug fixes before MVP (having this merged would complicate bug fixes)

@tomasciccola
Copy link
Contributor Author

I think we should not merge this until we have released the MVP, in case we need bug fixes before MVP (having this merged would complicate bug fixes)

wouldn't it make sense to have a 'develop' branch where we merge this so we don't have a bunch of dangling open PRs (but maybe there won't be many of this)?

EvanHahn
EvanHahn approved these changes Sep 30, 2024
View reviewed changes
Copy link
Contributor

@EvanHahn EvanHahn left a comment

Choose a reason for hiding this comment

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

LGTM other than one small nitpick.

proto/remoteDetectionAlert/v1.proto Outdated Show resolved Hide resolved
@tomasciccola tomasciccola merged commit bf8e0a9 into main Sep 30, 2024
6 checks passed
@tomasciccola tomasciccola deleted the feat/addRemoteDetectionAlert branch September 30, 2024 14:24
@optic-release-automation optic-release-automation bot mentioned this pull request Oct 2, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gmaclennan gmaclennan gmaclennan requested changes

@EvanHahn EvanHahn EvanHahn approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add alerts type to schema
3 participants
@tomasciccola @gmaclennan @EvanHahn