Skip to content

Use new location metadata fields#1002

Open
slifty wants to merge 1 commit into
mainfrom
1001-use-new-location-fields
Open

Use new location metadata fields#1002
slifty wants to merge 1 commit into
mainfrom
1001-use-new-location-fields

Conversation

@slifty
Copy link
Copy Markdown
Contributor

@slifty slifty commented Apr 27, 2026

This PR modifies the web app to use the new location metadata fields.

Resolves #1001

Copilot AI review requested due to automatic review settings April 27, 2026 19:37
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 27, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 49.93%. Comparing base (69c71c0) to head (07ca375).
⚠️ Report is 31 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1002      +/-   ##
==========================================
+ Coverage   49.54%   49.93%   +0.38%     
==========================================
  Files         346      346              
  Lines       11414    11421       +7     
  Branches     1944     1946       +2     
==========================================
+ Hits         5655     5703      +48     
+ Misses       5571     5528      -43     
- Partials      188      190       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Updates the Angular web app’s location handling to support newly introduced location metadata fields (e.g., name/sublocation/city and role/precision/raw metadata), aligning the UI formatting and client-side models with the updated location schema.

Changes:

  • Extend location DTO/model types with new metadata fields (name, sublocation, city, altitudeMeters, role/precision/raw metadata).
  • Map Stela location fields into the app’s LocnVOData shape, including renaming role/precision/raw to locationRole/locationPrecision/rawMetadata.
  • Prefer new location fields when rendering location display strings and when creating locations from Google Places.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
src/app/shared/services/api/record.repo.ts Adds new Stela location fields and maps role/precision/raw into the app’s renamed metadata fields during conversion.
src/app/shared/pipes/pr-location.pipe.ts Updates display formatting to prefer sublocation, city, and name when present.
src/app/models/locn-vo.ts Extends LocnVOData with new location metadata fields used across the app.
src/app/file-browser/components/location-picker/location-picker.component.ts Populates sublocation, city, and name when creating a location from a selected Google Place.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@slifty slifty force-pushed the 1001-use-new-location-fields branch 2 times, most recently from 8aacc88 to 0ef7893 Compare April 28, 2026 19:54
@slifty slifty requested a review from Copilot April 28, 2026 20:01
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/app/file-browser/components/location-picker/location-picker.component.ts Outdated
Comment thread src/app/shared/pipes/pr-location.pipe.ts Outdated
Comment thread src/app/shared/services/api/record.repo.ts
Comment thread src/app/file-browser/components/location-picker/location-picker.component.spec.ts Outdated
@slifty slifty force-pushed the 1001-use-new-location-fields branch from 0ef7893 to 75feaaa Compare April 29, 2026 19:11
@cecilia-donnelly cecilia-donnelly self-requested a review April 29, 2026 19:15
@slifty slifty force-pushed the 1001-use-new-location-fields branch from 75feaaa to e18814e Compare April 29, 2026 19:21
Copy link
Copy Markdown
Member

@cecilia-donnelly cecilia-donnelly left a comment

Choose a reason for hiding this comment

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

I don't see anything obviously off here! I have some questions, likely answered by your partner PRs to this one, but wanted to ask them right away.

return component
? component[getShortName ? 'short_name' : 'long_name']
: null;
return component ? component.long_name : null;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What was the short name?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@slifty is going to figure out if we want to keep that after all.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The idea is that short vs long name isn't part of the new location specification, and so we just use the long name now because it's the only thing that will exist in the future.

Short name might have been "CA" when long name was "California"

The backend is changing which location metadata fields to store and we
need to use those fields.

Issue #1001 Use new location metadata fields
@slifty slifty force-pushed the 1001-use-new-location-fields branch from e18814e to 07ca375 Compare April 29, 2026 20:13
Copy link
Copy Markdown
Contributor

@aasandei-vsp aasandei-vsp left a comment

Choose a reason for hiding this comment

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

LGTM, small observations.

...rest,
locnId: Number.parseInt(stelaLocation.id, 10),
adminOneName: state ?? undefined,
locationPrecision: precision ?? undefined,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

These checks don't really make sense.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's because precision can be null but locationPrecision is not nullable.

These lines are mapping null to undefined (so if precision is null it would be translated to undefined, which is allowed)

latitude?: number | null;
longitude?: number | null;
altitudeMeters?: number | null;
precision?: LocationPrecision | null;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why do we need them to be also null? they can just be skipped, as they are optional.

Copy link
Copy Markdown
Contributor Author

@slifty slifty May 5, 2026

Choose a reason for hiding this comment

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

null is a value and undefined is a lack of any value.

const foo = {
a: null,
b: undefined,
}

console.log(foo.a);
console.log(foo.b);
console.log(foo.c);

results in

null
undefined
undefined

null and undefined are both falsey, but in any case where stela could return null or provide no value at all we'd want the types to reflect both possibilities.

I do personally believe that it is better to never have optional properties in an API's entity shape (outside of patch), and instead have our API commit to there being a nullable value because of this very ambiguity!

All that said: I'll double check the stela API commitments. If this can be null we should have null, and if not we shouldn't!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

OK! So currently the stela spec lists these as optional but not nullable; ironically the implementation is the inverse (stela can return null, and always returns something).

Given that I think we should keep this as written. Null is a possible value, and in theory stela could choose to not return a given value.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Created a stela ticket about this: PermanentOrg/stela#742

@slifty slifty added the QA This issue is ready for QA / user acceptance testing label May 5, 2026
@slifty slifty requested a review from omnignorant May 5, 2026 19:13
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 5, 2026

QA Instructions

QA Testing Instructions

Summary

This pull request modifies the application to adopt new location metadata fields for both the LocationPickerComponent and the backend integration (record.repo). It also removes references to old/deprecated fields that will no longer be supported. Unit tests have been added to validate the changes.

Test Environment Setup

  1. Ensure you have a functional development environment for the web app, with necessary dependencies (npm install).
  2. Verify the backend is updated to match the new metadata field structure.
  3. Start the application (npm run start) and run all tests (npm run test) to confirm no initial issues.

Test Scenarios

1. Validate Location Metadata Mapping in LocationPickerComponent
  • Steps:
    1. Open the place-picker UI in the application.
    2. Select a location using Google Places autocomplete.
    3. Verify that a LocnVOData object is created with the following field mappings:
      • latitude and longitude should match the geometry values from Google Places.
      • sublocation should be a combination of street_number and route or null if neither exists.
      • city, adminOneName, country, and postalCode should map to their respective components (e.g., locality for city).
      • Deprecated fields like streetNumber, streetName, and countryCode should not appear.
  • Expected Results:
    • The object should match the documented field transformations.
    • The UI should not display any error and should properly render the selected location details.
2. Test Edge Cases for createLocnFromPlace
  • Steps:
    1. Trigger the createLocnFromPlace() method programmatically in LocationPickerComponent.
    2. Pass in test cases using the provided helper functions (buildPlace, buildAddressComponents):
      • No street_number or route provided.
      • Only route provided.
      • Place name already matches the sublocation.
      • Complex edge cases where certain components (e.g., adminOneName) are missing.
    3. Confirm the outputs comply with the test case expectations in the updated spec file.
  • Expected Results:
    • Components are robust and handle null/missing values gracefully without breaking.
    • Correct values are assigned for all edge cases as specified in the tests.
3. Validate Integration with Backend API for Location Fields
  • Steps:
    1. Add or update an object via the app backend that includes location information.
    2. Verify that the API submits and retrieves fields using the updated schema:
      • Ensure sublocation, city, state, postalCode, country, latitude, and longitude are used.
      • Confirm legacy fields (e.g., streetNumber, streetName, countryCode, etc.) are excluded.
    3. Check the database or network logs to ensure compliance with the new field schema.
  • Expected Results:
    • API correctly handles and returns only the new fields as defined.
    • Existing records without old fields should still function properly without any errors.
4. Regression Test for Existing Features
  • Areas to Test:
    • Location selection and display in any dependent UI components.
    • Backend integrations that consume or process location metadata.
    • Location-related search or filtering functionality in the app.
  • Expected Results:
    • No regressions or broken functionality across the app.
    • All components and features using location metadata should work seamlessly.

Regression Risks

  • High risk in areas that depend on the previous implementation of location metadata, including backend API calls and location-dependent UI components.
  • Ensure backward-compatibility has been accounted for in all cases where legacy data may exist.

Things to Watch For

  1. Backward Compatibility:
    • Ensure locations stored before this change are properly handled without errors.
  2. Google Maps API Changes:
    • Verify the assumptions about Google Places API results (e.g., address components) remain valid.
  3. Edge Cases:
    • Missing metadata or incomplete address components can cause unexpected results if not tested thoroughly.

Additional Notes

  • Unit tests in location-picker.component.spec.ts and record.repo.spec.ts thoroughly cover logic updates. Confirm all tests pass successfully.
  • Review existing e2e tests or add new ones if gaps are identified.

Generated by QA Instructions Action

@slifty slifty removed QA This issue is ready for QA / user acceptance testing labels May 5, 2026
@slifty
Copy link
Copy Markdown
Contributor Author

slifty commented May 5, 2026

In my own testing: I'd like to wait until the stela endpoints for location creation are live before moving the frontend to use the new properties.

Copy link
Copy Markdown
Member

@omnignorant omnignorant left a comment

Choose a reason for hiding this comment

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

Not 100% sure what I am testing or what the success condition should be, but I did my best to suss it out of the QA guide. The gist seems to be "does a location properly save using the current picker UI"

Location editing seems to work inconsistently. In the screen capture attached here, I seem to have determined the following:

  • location seems to save on initial selection
  • for records without existing location data, the changed metadata persists after a refresh;
  • for records with existing location data, the metadata does not persist;
  • location metadata in profile, both general and milestone persists.
Screen.Recording.2026-05-08.at.4.50.36.PM.mov

@slifty
Copy link
Copy Markdown
Contributor Author

slifty commented May 11, 2026

@omnignorant sorry for the confusion -- I pulled this off of the QA pile since we need to do some additional changes for everything to work as expected!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Use new location metadata fields

5 participants