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.

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

Add username to features #1324

Merged
merged 8 commits into from
Sep 25, 2023

Conversation

openbrian
Copy link
Contributor

For #430

Will look like this.

{
  "type": "FeatureCollection",
  "features": [
    {
      "type": "Feature",
      "properties": {
        "name": "K",
        "description": "K",
        "owner": 2
      },
      "geometry": {
        "type": "Point",
        "coordinates": [
          -1.911621,
          54.72462
        ]
      }
    }
  "_umap_options": {
    "displayOnLoad": true,
    "browsable": true,
    "name": "Layer 1"
  }
}

@yohanboniface
Copy link
Member

Given this property is not going to be editable by users, I'm not why we'd use the form to set it. Why not just setting it in the FeatureMixin.initialize method ? :)

Only add the owner property to defaultProperties, if there is a user.  In other words the user can use the map without logging in.
@openbrian
Copy link
Contributor Author

Given this property is not going to be editable by users, I'm not why we'd use the form to set it. Why not just setting it in the FeatureMixin.initialize method ? :)

Right. It's already being set by FeatureMixin.initialize. I removed the form part. The owner property continues to be sent in the geojson.

I added some support for anonymous users. Let me know if that will work for you.

@yohanboniface
Copy link
Member

Sounds good!

Let's add a global map option to opt in this, controlled by a django settings for now (we'll see to expose it to all users later). Something like options.trackUsers and settings.UMAP_TRACK_USERS, wdyt ?

@openbrian
Copy link
Contributor Author

First, I feel "trackUsers" is a lightning rod that should be avoided. Let's call this featuresHaveOwners. I think it's fine to add an option... But I'm getting dizzy with finding where to initialize it!

Currently I have this UMAP_DEFAULT_FEATURES_HAVE_OWNERS in base.py. The MapDetailMixin version of get_geojson() will set this

{
  "properties": {
    "features_have_owners": settings.UMAP_DEFAULT_FEATURES_HAVE_OWNERS
  }
}

I added a field in "Edit map settings" / "Default properties". But I don't know how to get my map option to set that widget.

@yohanboniface
Copy link
Member

featuresHaveOwners

setFeaturesOwners ?

The MapDetailMixin version of get_geojson() will set this

I'd add it in the MapDetailMixin.get_context_data

I added a field in "Edit map settings" / "Default properties". But I don't know how to get my map option to set that widget.

No need for that yet, I'd say. A Django setting is enough for this step.

Only add the owner property if the feature flag (map.options.featuresHaveOwner) is enabled.

Default to features do NOT have owners.
@openbrian
Copy link
Contributor Author

Got the map default properties form field working. Issue was just a typo. If you really want it removed let me know.

I think this is a working version. I'm able to toggle the base.py setting and see a difference in the feature properties. Please review code and/or test.

@yohanboniface
Copy link
Member

Looks good to me!

My only concern is about the naming. @davidbgk any alternative suggestion, or should we go as is ? :)

@davidbgk
Copy link
Contributor

I wonder if adding that very specific property, we hide the fact that we need a way to let instance's owners enrich the data with custom properties. Maybe some kind of basic plugin architecture to achieve that would be more useful to the whole community.

I'm afraid that we have to repeat the current process and end up with legacy properties that we have to deal with for the rest of uMap's life 😅

Copy link
Contributor

@davidbgk davidbgk left a comment

Choose a reason for hiding this comment

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

After discussion with @yohanboniface, let's make that first step, thanks a lot for your contribution @openbrian

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.

None yet

3 participants