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

Improve consistency of field titles, labels, types across the schema #19531

Merged
merged 5 commits into from
Feb 6, 2021
Merged

Improve consistency of field titles, labels, types across the schema #19531

merged 5 commits into from
Feb 6, 2021

Conversation

ahed-compucorp
Copy link
Contributor

@ahed-compucorp ahed-compucorp commented Feb 4, 2021 •
edited by colemanw
Loading

Overview

The purpose of this PR is to normalize field titles, labels & widgets across the schema to follow these conventions:

  • The ID field should have <html><type>Number</type></html>
  • For FK fields (which contain the ID of another entity), <title> should end with "ID" and <html><label> should not. E.g. Address.contact_id field should have title "Contact ID" and label "Contact".
  • Titles and labels should not contain the name of its own entity. E.g. Email.contact_id should have the title "Contact ID" not "Email Contact ID" and the label should be "Contact" not "Email Contact".

@civibot
Copy link

civibot bot commented Feb 4, 2021

(Standard links)

@civibot civibot bot added the master label Feb 4, 2021
@seamuslee001 seamuslee001 self-assigned this Feb 4, 2021
seamuslee001
seamuslee001 reviewed Feb 5, 2021
View reviewed changes
Copy link
Contributor

@seamuslee001 seamuslee001 left a comment

Choose a reason for hiding this comment

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

These seem to make sense but would note that ./setup.sh -g would need to be run as well

@ahed-compucorp ahed-compucorp changed the title Add html tag to all files in xml/schema [WIP] Add html tag to all files in xml/schema Feb 5, 2021
@colemanw colemanw mentioned this pull request Feb 5, 2021
Merged
@colemanw
Copy link
Member

colemanw commented Feb 5, 2021

Conflicting files
xml/schema/Campaign/CampaignGroup.xml
xml/schema/Campaign/Survey.xml
xml/schema/Contribute/ContributionPage.xml
xml/schema/Event/Participant.xml

@colemanw colemanw self-requested a review February 5, 2021 16:49
@ahed-compucorp ahed-compucorp force-pushed the master branch 3 times, most recently from 1a83112 to 73440bf Compare February 5, 2021 21:49
@eileenmcnaughton
Copy link
Contributor

Argh - we should have merged quickly while it was unconflicted & passing - looks like it went stale!

@ahed-compucorp this is obviously a tricky PR to manage since it's inclined to go stale & run into test issues. Maybe if you drop out the last commit for now it will be passing again & we can move more quickly to merge

I'm happy to leave the DAO generation @seamuslee001 mentioned until all is merged

@colemanw
Copy link
Member

colemanw commented Feb 6, 2021

@eileenmcnaughton MCs have been fixed but there are some test failures due to tests expecting certain field titles to be returned.
@ahed-compucorp is working on updating the tests.

colemanw
colemanw approved these changes Feb 6, 2021
View reviewed changes
Copy link
Member

@colemanw colemanw left a comment

Choose a reason for hiding this comment

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

All these updates look good to me & will help the Search Kit UI feel consistent.

@ahed-compucorp
Copy link
Contributor Author

@eileenmcnaughton I can regenerate DAO files with ./setup.sh -g as @seamuslee001 mentioned

I was feeling a bit overwhelmed by how DAO files are generated from theses xml files in xml/schema.

@ahed-compucorp ahed-compucorp force-pushed the master branch 3 times, most recently from 7bbffe7 to a4b48f7 Compare February 6, 2021 13:09
@colemanw
Copy link
Member

colemanw commented Feb 6, 2021

This looks perfect. Thank you @ahed-compucorp

@colemanw colemanw changed the title [WIP] Add html tag to all files in xml/schema Improve consistency of field titles, lables, types across the schema Feb 6, 2021
@colemanw colemanw changed the title Improve consistency of field titles, lables, types across the schema Improve consistency of field titles, labels, types across the schema Feb 6, 2021
@colemanw colemanw merged commit 4a5987a into civicrm:master Feb 6, 2021
@eileenmcnaughton
Copy link
Contributor

Great work @ahed-compucorp !

@mattwire
Copy link
Contributor

mattwire commented Feb 8, 2021

@colemanw This PR has caused a regression #19531 - for example editing a paymentprocessor gives a fatal error "Unsupported html-element '' "

@colemanw
Copy link
Member

colemanw commented Feb 8, 2021

@mattwire can you please try this fix: #19560

@ahed-compucorp ahed-compucorp mentioned this pull request Feb 9, 2021
Merged
@colemanw colemanw mentioned this pull request Mar 4, 2021
Merged
agileware-justin
agileware-justin reviewed Apr 16, 2021
View reviewed changes
CRM/Core/DAO/Website.php
'description' => ts('FK to Contact ID'),
'where' => 'civicrm_website.contact_id',
'table_name' => 'civicrm_website',
'entity' => 'Website',
'bao' => 'CRM_Core_BAO_Website',
'localizable' => 0,
'FKClassName' => 'CRM_Contact_DAO_Contact',
'html' => [
'label' => ts("Contact"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Apologies in advise for commenting on a merged PR and spamming everyone.

But I was trying to track down a field label inconsistency in the Data Processor, https://lab.civicrm.org/extensions/dataprocessor/-/issues/90

Why is this field label "Contact" and not "Contact ID" ?

Copy link
Member

Choose a reason for hiding this comment

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

The difference between "title" and "label" is that the title describes what the field contains (Contact ID) and the label is how a form label would appear (a Select field labeled "Contact").

Copy link
Contributor

Choose a reason for hiding this comment

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

cool thanks @colemanw

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

@agileware-justin agileware-justin agileware-justin left review comments

@seamuslee001 seamuslee001 seamuslee001 left review comments

@colemanw colemanw colemanw approved these changes

Assignees

@seamuslee001 seamuslee001

Labels
comp:search-kit master merge on pass
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ahed-compucorp @colemanw @eileenmcnaughton @mattwire @seamuslee001 @agileware-justin