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

feature: nickname field added #7185

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

etopipec
Copy link

Description & Issue number it closes

Nickname field added.
#6651

Screenshots (if appropriate)

20241014_19h56m20s_grim

How to test the changes?

Visual inspection.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Visual inspection. Creating, editing person. Adding nickname.

@etopipec etopipec requested a review from a team as a code owner October 14, 2024 13:13
@etopipec etopipec requested review from respencer, DawoudIO, grayeul, DAcodedBEAT, MrClever and bigtigerku and removed request for a team October 14, 2024 13:13
Copy link
Contributor

@DawoudIO DawoudIO left a comment

Choose a reason for hiding this comment

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

I would like to understand why we can do it via custom fields

If we want to add a db field I'm ok with it but we need to regen the orm and do a few more things

@etopipec
Copy link
Author

I will study the issue of custom fields and will definitely come back

@etopipec
Copy link
Author

I found two tables. If I understood you correctly, we need to add a new record to person_custom_master, and add a new column to person_custom. If I'm wrong, please correct me.
20241015_07h03m37s_grim

@DAcodedBEAT
Copy link
Contributor

DAcodedBEAT commented Oct 16, 2024

I would like to understand why we can do it via custom fields

If we want to add a db field I'm ok with it but we need to regen the orm and do a few more things

@DawoudIO This is your project, but I disagree with the idea of custom/dynamic properties, especially for common fields.

In general, dynamic columns in SQL is generally a bad idea for these reasons:

  1. Schema Inconsistency: While the app may not change often, having different columns across deployments makes it harder to maintain consistency. It increases the risk of errors when deploying updates or running queries across environments.
  2. Difficult to Scale: If you ever need to scale the app or share the database across different instances (I.e. the ability to support multiple churches), managing variable schemas becomes much more complicated.
  3. Maintenance Overhead: Even if columns don’t change frequently, tracking which deployment uses which schema creates additional maintenance work. Debugging or updating the app gets harder over time.
  4. Upgrade Challenges: When deploying new versions or updates, migrations and schema changes can become unpredictable, making rollouts riskier and harder to automate.

@MrClever
Copy link
Collaborator

I'm inclined to agree with @DawoudIO on this one; a "nickname" field (or any other arbitrary attribute) can be added by the admin via leveraging custom fields, without us needing to make code changes. Sure, dynamic database stuff introduces some complexity with schema changes (as @DAcodedBEAT pointed out) but I believe this is the lesser of the two evils.

Historically, when support requests have come through for random fields to be added to person/family pages the discussion is usually that whatever the requested attribute is, represents a corner case very few people would actually want or need it. In which case, the custom field feature still allows the admin to create whatever they like and the world keeps spinning.

The next phase of the discussion inevitably centres around being able to "hide" arbitrary fields that admins may not want to see. Deleting them isn't really an option because they are part of the schema etc, but we could simply hide them from the UI. However the current code base has no way to do this (yet) although it would be an interesting project.

Personally, I have no use for a "nickname" field, and would loath having to look at another empty field on an already busy page.

My $0.02 worth 👍🏻

@DAcodedBEAT
Copy link
Contributor

I'm inclined to agree with @DawoudIO on this one; a "nickname" field (or any other arbitrary attribute) can be added by the admin via leveraging custom fields, without us needing to make code changes. Sure, dynamic database stuff introduces some complexity with schema changes (as @DAcodedBEAT pointed out) but I believe this is the lesser of the two evils.

Historically, when support requests have come through for random fields to be added to person/family pages the discussion is usually that whatever the requested attribute is, represents a corner case very few people would actually want or need it. In which case, the custom field feature still allows the admin to create whatever they like and the world keeps spinning.

The next phase of the discussion inevitably centres around being able to "hide" arbitrary fields that admins may not want to see. Deleting them isn't really an option because they are part of the schema etc, but we could simply hide them from the UI. However the current code base has no way to do this (yet) although it would be an interesting project.

Personally, I have no use for a "nickname" field, and would loath having to look at another empty field on an already busy page.

My $0.02 worth 👍🏻

@MrClever I see your point about avoiding clutter, but:

  1. there is already a lot of unnecessary clutter all across this database schema
  2. the custom fields contributes to this clutter and
  3. since non-official names (like nicknames, given names vs legal/government names, etc) are very common, especially in community-oriented systems like church management software, they could be valuable as a base user property.

As a base user property, this would provide consistency across deployments and improve personalization for users who prefer being addressed informally, while keeping it optional for those who don’t need it. It simplifies maintenance by avoiding custom fields and gives flexibility to scale with user needs in the future.

Copy link
Contributor

github-actions bot commented Dec 5, 2024

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@github-actions github-actions bot added the Stale label Dec 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants