Skip to content

Ignore location displayName#1025

Merged
slifty merged 1 commit into
mainfrom
1024-location-display-name
May 26, 2026
Merged

Ignore location displayName#1025
slifty merged 1 commit into
mainfrom
1024-location-display-name

Conversation

@slifty

@slifty slifty commented May 15, 2026

Copy link
Copy Markdown
Contributor

This PR removes recognition of the displayName attribute of a location. This is a legacy field we are about to remove, but also it wasn't actually being stored which resulted in unexpected behavior (specifically, the value disappeared on refresh for users)

Resolves #1024

Copilot AI review requested due to automatic review settings May 15, 2026 16:35
@codecov

codecov Bot commented May 15, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 50.00%. Comparing base (540f329) to head (e6b0075).
⚠️ Report is 6 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1025      +/-   ##
==========================================
+ Coverage   49.99%   50.00%   +0.01%     
==========================================
  Files         348      348              
  Lines       11508    11502       -6     
  Branches     1978     1975       -3     
==========================================
- Hits         5753     5752       -1     
+ Misses       5565     5564       -1     
+ Partials      190      186       -4     

☔ 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.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR removes support for the legacy displayName field on location objects so the UI no longer shows an ephemeral value that disappears on refresh (per Issue #1024). The change aligns the frontend’s location model, formatting pipe, and location picker UI around only persisted location fields.

Changes:

  • Removed displayName from the LocnVOData model shape.
  • Updated PrLocationPipe to stop emitting/concatenating a displayName into the formatted location output.
  • Updated the location picker to stop setting and rendering displayName.

Reviewed changes

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

File Description
src/app/shared/pipes/pr-location.pipe.ts Removes displayName from the pipe output and stops injecting it into the formatted full string.
src/app/models/locn-vo.ts Removes legacy displayName from the location VO data interface.
src/app/file-browser/components/location-picker/location-picker.component.ts Stops populating locn.displayName from Google Places place.name.
src/app/file-browser/components/location-picker/location-picker.component.html Stops rendering currentLocationDisplay.displayName in the location panel.

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

@aasandei-vsp aasandei-vsp left a comment

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.

Code wise, the change seems correct. But I have some issues understanding what has changed and where in the UI.

  • Where exactly did the displayName appear in the UI?
  • Can you give me some steps to reproduce before and after?

This is the research I have done:

A. Location on sidebar(preview and full) On main and also on the branch I had the exact same result. Steps I tried:

  1. Select a file
  2. Click on "Click to set location"
  3. In the search bar, type Breazu.
  4. Select "Breazu, Romania, RO"
  5. Click save
    EXPECTED: The map updates the pin, the location line says "Breazu, Romania, RO"
    ACTUAL: As expected.
    ?????? Not sure where the display name should be present, taking into account that I saw the same thing on both main and this branch.
  6. Refresh the page.
    EXPECTED: The location should show "Breazu, Romania, RO" and the pin on the map should be correct.
    ACTUAL: As expected.
  7. Follow steps 2-6 again, but this time type "Iasi" in the search bar
    EXPECTED: The location should show "Iași, IS, RO, RO" and the pin on the map should be correct.
    ACTUAL: The location shows "Breazu, Romania, RO" and the pin on the map is for this location.

B. Location pipe
I have seen it used in the code in 2 places, on the public profile and somewhere in shared, but I cannot get to them in the UI. Could you help me with some steps for that, please?

@slifty

slifty commented May 22, 2026

Copy link
Copy Markdown
Contributor Author

@aasandei-vsp I believe this only applies to named locations (e.g. landmarks such as "Eiffel Tower"). The landmark name would appear pre-save:

image

but then disappear on refresh:

image

Within this branch the landmark name is not rendered at all, just the address.

As for the location pipe, I'll look into it and try to get you instructions there.

@aasandei-vsp

aasandei-vsp commented May 25, 2026

Copy link
Copy Markdown
Contributor

@slifty Ah, thank you! And yes, this specific case works as expected, both in the sidebar and profile.

I've also figured out the location (there are actually 2 places that I've found) on the profile where you can edit:
Milestones location:
https://www.loom.com/share/837624ecab33442ba62758e0804b1264
Location Established from group information:
https://www.loom.com/share/5c49cb82c0ef48209f65acf6a0dc8c62

But also where these edits appear:
https://www.loom.com/share/9f8292ce708e46df943e8d7de54361fd

The only thing remaining is the fact that in the sidebar, after the first time you set the location, you cannot update it, but I am not sure it's relevant to this task.

I'll approve this PR, because the bug mentioned above is irelevant in this context, but we need to create a bug for it and maybe fix it before we start the UI work on location.

The display name of a location was populated by the `name` returned by
our location lookup.  Some locations (e.g. landmarks) have names, which
is where that came from.  We don't actually save this information, which
means users would see it at first but it would disappear on refresh.

Rather than save it we're opting to remove the concept entirely.

Issue #1024 Do not render the displayName of a location
@slifty slifty force-pushed the 1024-location-display-name branch from ef0259a to e6b0075 Compare May 26, 2026 19:00
@slifty slifty added the QA This issue is ready for QA / user acceptance testing label May 26, 2026
@slifty

slifty commented May 26, 2026

Copy link
Copy Markdown
Contributor Author

@cecilia-donnelly is going to give this a QA pass!

@github-actions

Copy link
Copy Markdown
Contributor

QA Instructions

QA Testing Instructions for PR: Ignore location displayName


Summary

This Pull Request removes all references to the deprecated displayName property of the LocnVOData model. Previously, this field would hold the name returned by the location lookup but was not being saved, leading to inconsistency. To address this, the displayName field is no longer used or rendered in associated components or pipes. The goal is to streamline and avoid misleading users with disappearing data.


Test Environment Setup

No additional setup is required beyond the standard development environment. Ensure that the application builds successfully and that all components load properly.


Test Scenarios

  1. Location display in the Location Picker component:

    • Steps:
      1. Navigate to a feature that uses the LocationPickerComponent (e.g., the file browser).
      2. Open the location picker dialog.
      3. Select a location with a displayName value in the lookup data.
      4. Observe the location information displayed in the "location info" area of the location picker.
    • Expected Result:
      • The displayName value is not shown in the location info panel.
      • The location data correctly displays other fields, such as line1 and line2.
  2. Refresh behavior after selecting a new location:

    • Steps:
      1. Select a location in the location picker and confirm/save changes.
      2. Refresh the page and reopen the location picker.
    • Expected Result:
      • The selected location persists as expected, and there are no missing or unexpected fields in the location detail.
  3. Regression: Location lookup functionality:

    • Steps:
      1. Verify that location lookups still function as expected (e.g., by typing into the location search).
      2. Confirm that suggested locations are displayed correctly and can be selected without errors.
    • Expected Result:
      • Location selection and auto-completion work as intended.
      • No errors are encountered in the console during this process.
  4. Regression: PrLocationPipe output without displayName:

    • Steps:
      1. Test PrLocationPipe transformations using LocnVOData objects where displayName was previously populated. (If a unit test exists for the pipe, ensure the tests pass.)
      2. Verify that line1, line2, and full properties are populated based on other location data (e.g., streetName and country).
    • Expected Result:
      • The PrLocationPipe functions correctly and returns outputs without relying on the displayName property.
  5. Regression: Location model usage (LocnVOData):

    • Steps:
      1. Audit all features that consume LocnVOData.
      2. Check for any errors or broken functionality related to the removal of the displayName property.
    • Expected Result:
      • Features relying on LocnVOData continue to function as expected.
      • No errors in the console related to the displayName field.

Regression Risks

  • The removal of the displayName property may affect any overlooked instances where it was still in use.
  • The modification to PrLocationPipe should be closely monitored since this is a shared service used across multiple features.

Things to Watch For

  • Ensure that selection, persistence, and display of location data remain fully functional and unaffected.
  • Watch for unexpected console errors or broken references to the displayName property in any impacted areas outside those explicitly changed in the diff.
  • Test locations without line1 or line2 values to confirm the absence of displayName doesn't negatively affect the output formatting.

Additional Notes

  • Verify that automated tests (if any) have been updated to reflect the removal of the displayName field.
  • Consider writing additional unit tests for the PrLocationPipe to validate the expected output in the absence of displayName.

Generated by QA Instructions Action

@cecilia-donnelly cecilia-donnelly left a comment

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.

I checked in the sidebar and in the profile. Thanks!

@slifty slifty merged commit aaef917 into main May 26, 2026
17 checks passed
@slifty slifty deleted the 1024-location-display-name branch May 26, 2026 21:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

QA This issue is ready for QA / user acceptance testing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Do not render the displayName of a location

4 participants