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

Update default form definitions for Geo #5367

Open
wants to merge 74 commits into
base: production
Choose a base branch
from

Conversation

grantfitzsimmons
Copy link
Member

@grantfitzsimmons grantfitzsimmons commented Nov 1, 2024

Fixes #5311, #5073, #3120, #2153, #1515, #584, #364, specify/specify6#1210, specify/specify6#1213, specify/specify6#1229, specify/specify6#1232, #4591, #5020, #2016 and specify/specify6#1223

This PR is intended to update underlying app resources necessary for the geology discipline as well as improve other defaults where possible.

Geo-focused Improvements

CollectionObject form for Geology

image image

Common Views

RelativeAge

image

Added AbsoluteAge form

image

Added TectonicUnit form

image

Added TectonicUnitTreeDef form

image

Added TectonicUnitTreeDefItem form

image

Major Improvements

Type Searches

  • You can now search for Collection, Institution, and SpecifyUser, improving a number of places around the app!
  • Instructions for users modifying this file are now included in the app resource itself

Record Sets

image
  • You can now transfer ownership of a record set to another user!

Agent

Screen.Recording.2024-11-07.at.9.56.15.AM.mov
  • Person is now the default value (huge improvement)
  • Only when the Agent is type Group will the members subview display
  • Agent Identifiers now displays properly, and subview is included on the default form

Taxon

hybridFieldsConditional.mov
  • Form is now conditional, displaying HybridParent fields only when IsHybrid is checked

Collection Relationships

Old form:

image

New form:

image
  • Can now easily create a Collection Relationship via the form
  • Using the updated TypeSearches, "Collection" can be searched and displayed properly

App Resources

AppResource

image

Reports

image
  • You can now easily find who owns the query and what the query is named from the app resource editor for a given report or label!
  • The Reports subview button only appears when the mimetype is jrxml/label
  • New SpReport form for the first time

SpViewSetObj

image

Pick List

Old form:

image

New form:

image
  • Removed redundant header
  • Now users can remove the pick list limit by setting it to -1

Tree Definition

Old Tree Definition form:

image

New Tree Definition form:

image
  • Can now change full name direction for all trees

Miscellaneous Improvements

  • Preparation countAmt can no longer be set to a negative number by default
  • Default form definition files and DataObjFormatters have finally been reformatted by the built-in Specify 7 system
  • Removed a number of invalid fields on all default forms
  • Updated hard-coded field labels to use schema-assigned captions instead

Testing instructions

THIS MUST BE TESTED LOCALLY UNTIL THE TEST PANEL SUPPORTS STATIC FILES

Okay, so this PR is big, but it is almost entirely XML changes. This touches a lot of the default forms, so I have a big ask– please test the following XML resources in Specify. You are likely to find issues that aren't related to this PR which I am happy to resolve, so report them. If it grows outside the scope of this, I'll open another and we'll resolve it in the future.

Remember to delete any custom form definitions / app resources before testing this as those will be used first.

  • Create a new DataObjFormatters app resource and verify that all table formats and aggregations appear as expected
  • Build queries and return the (formatted) and (aggregated) results of as many tables as you can, with a focus on new Geo tables. If a table format or aggregation is missing where it should be included, let me know. Some tables (e.g. *property, *attribute, *groupjoin) do not have a table format or aggregation defined as it is ambiguous as to which fields we should display.
  • Create a new TypeSearches app resource and verify that it is working as expected. Delete this resource after it is populated as you should be using the default.
  • Open the CollectionRelType form and search for a collection. You should be able to type the name of a collection and select it. You will encounter HierarchyException when loading Collectionreltype #4989, but that is not in the scope of this issue, and you can safely dismiss it.
  • Create an ExportFeed app resource and verify that you can search for a user in the database by name. You should see the name appear correctly and you should be able to search for a user in the query combo box without the 🔍 QB function.
  • Create a new WebLinks app resource and verify that all web links (~5) appear and work as expected. See this guide for setting up a conditional form on AgentIdentifier to leverage those new web links.
  • Verify that the geology.views.xml can be used as the basis of a new form (create a new form definition and select Geology). Let me know what you think. This is subjective to some extent, and I welcome suggested changes (especially via XML snippets).
  • Test that these new view definitions (mostly shown in this issue's body) are displayed properly (compare to defaults on production):
    • CollectionRelType
      • Verify that you can create a Collection Relationship without the query builder (ignoring this error)
    • Agent should default to Person instead of Organization finally!
      • Agent conditional: If the agent is not a Group, the Members subview should not be shown.
    • TectonicUnit
    • TectonicUnitTreeDef
    • TectonicUnitTreeDefItem
    • TaxonTreeDef
    • Taxon
      • Taxon conditional: Verify that when you check IsHybrid the fields HybridParent1 and HybridParent2 appear. When it is not checked, they should not appear.
    • GeographyTreeDef
    • StroageTreeDef
    • LithoStratTreeDef
    • GeologicTimePeriodTreeDef
    • RelativeAge
    • AbsoluteAge
    • PickList
      • Set the size limit to -1 and verify that you can add any number of pick list items (>500– and you can use the workbench to add the first 500)
    • PickListItem
    • SpAppResource (click pencil next to the title of an app resource)
      • SpAppResource conditional (should show Reports subview button if the resource is a report/label)
        • SpReport form should show ownership, associated query, title, in a read-only view
    • SpViewSetObj (click pencil next to the title of a form definition)
    • SpQuery (click pencil next to a query title)
      • Verify that you can change the ownership of a query
    • RecordSet (click pencil next to a record set title)
      • Verify that you can change the ownership of a record set
    • AgentIdentifier – navigate directly to the form via the data entry menu or by modifying the URL. It should no longer be auto-generated

Restructures common.views.xml for the first time in years based on the linting done by Specify 7

Fixes #5311
@grantfitzsimmons grantfitzsimmons added the 2 - Forms Issues that are related to the form system label Nov 1, 2024
@grantfitzsimmons grantfitzsimmons added this to the 7.9.8 milestone Nov 1, 2024
@CarolineDenis CarolineDenis modified the milestones: 7.9.8, 7.9.9 Nov 6, 2024
@grantfitzsimmons grantfitzsimmons marked this pull request as ready for review November 6, 2024 20:29
@grantfitzsimmons grantfitzsimmons marked this pull request as draft November 6, 2024 20:29
Fixes #2153
@grantfitzsimmons grantfitzsimmons linked an issue Nov 6, 2024 that may be closed by this pull request
Copy link
Contributor

@alesan99 alesan99 left a comment

Choose a reason for hiding this comment

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

Parent COG is appearing 👍

image

The new conditionals are also working

image

image

@grantfitzsimmons grantfitzsimmons requested review from a team and sharadsw November 25, 2024 15:34
Copy link
Contributor

@pashiav pashiav left a comment

Choose a reason for hiding this comment

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

Testing instructions

  • Create a new DataObjFormatters app resource and verify that all table formats and aggregations appear as expected
  • Build queries and return the (formatted) and (aggregated) results of as many tables as you can, with a focus on new Geo tables. If a table format or aggregation is missing where it should be included, let me know. Some tables (e.g. *property, *attribute, *groupjoin) do not have a table format or aggregation defined as it is ambiguous as to which fields we should display.
  • Create a new TypeSearches app resource and verify that it is working as expected. Delete this resource after it is populated as you should be using the default.
  • Open the CollectionRelType form and search for a collection. You should be able to type the name of a collection and select it. You will encounter HierarchyException when loading Collectionreltype #4989, but that is not in the scope of this issue, and you can safely dismiss it.
  • Create an ExportFeed app resource and verify that you can search for a user in the database by name. You should see the name appear correctly and you should be able to search for a user in the query combo box without the 🔍 QB function.
  • Create a new WebLinks app resource and verify that all web links (~5) appear and work as expected. See this guide for setting up a conditional form on AgentIdentifier to leverage those new web links.
  • Verify that the geology.views.xml can be used as the basis of a new form (create a new form definition and select Geology). Let me know what you think. This is subjective to some extent, and I welcome suggested changes (especially via XML snippets).
  • Test that these new view definitions (mostly shown in this issue's body) are displayed properly (compare to defaults on production):
    • CollectionRelType
      • Verify that you can create a Collection Relationship without the query builder (ignoring this error)
    • Agent should default to Person instead of Organization finally!
      • Agent conditional: If the agent is not a Group, the Members subview should not be shown.
    • TectonicUnit
    • TectonicUnitTreeDef
    • TectonicUnitTreeDefItem
    • TaxonTreeDef
    • Taxon
      • Taxon conditional: Verify that when you check IsHybrid the fields HybridParent1 and HybridParent2 appear. When it is not checked, they should not appear.
    • GeographyTreeDef
    • StroageTreeDef
    • LithoStratTreeDef
    • GeologicTimePeriodTreeDef
    • RelativeAge
    • AbsoluteAge
    • PickList
      • Set the size limit to -1 and verify that you can add any number of pick list items (>500– and you can use the workbench to add the first 500)
    • PickListItem
    • SpAppResource (click pencil next to the title of an app resource)
      • SpAppResource conditional (should show Reports subview button if the resource is a report/label)
        • SpReport form should show ownership, associated query, title, in a read-only view
    • SpViewSetObj (click pencil next to the title of a form definition)
    • SpQuery (click pencil next to a query title)
      • Verify that you can change the ownership of a query
    • RecordSet (click pencil next to a record set title)
      • Verify that you can change the ownership of a record set
    • AgentIdentifier – navigate directly to the form via the data entry menu or by modifying the URL. It should no longer be auto-generated


Issues/Comments

WOW looks soooo much better!

Using db ciscollections_2_15_24 to test.

1.

  • Open the CollectionRelType form and search for a collection. You should be able to type the name of a collection and select it. You will encounter HierarchyException when loading Collectionreltype #4989, but that is not in the scope of this issue, and you can safely dismiss it.

&

  • CollectionRelType
    - [ ] Verify that you can create a Collection Relationship without the query builder (ignoring this error)

No results pop up when I type in a name of a collection. I can only select the collection through query builder.

Screen.Recording.2024-11-26.at.11.48.55.AM.mov

2.

  • Verify that the geology.views.xml can be used as the basis of a new form (create a new form definition and select Geology). Let me know what you think. This is subjective to some extent, and I welcome suggested changes (especially via XML snippets).

I think it's a great base to work off of! It's simple and looks nice.


3.

  • SpAppResource conditional (should show Reports subview button if the resource is a report/label)

The Reports subview button shows up for Label resources, but does not for Report resources.

Screen.Recording.2024-11-26.at.12.47.51.PM.mov

4.

  • SpReport form should show ownership, associated query, title, in a read-only view

Also is not read-only on my db!

5.

  • Verify that you can change the ownership of a query

I can change ownership, but only through query builder. I cannot select users by searching their name in the field.

Screen.Recording.2024-11-26.at.1.00.37.PM.mov

See that I can change the ownership of a record set through searching in the field:

Screen.Recording.2024-11-26.at.1.06.01.PM.mov

@pashiav pashiav requested a review from a team November 26, 2024 19:50
Copy link
Collaborator

@combs-a combs-a left a comment

Choose a reason for hiding this comment

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

Testing instructions

THIS MUST BE TESTED LOCALLY UNTIL THE TEST PANEL SUPPORTS STATIC FILES

Okay, so this PR is big, but it is almost entirely XML changes. This touches a lot of the default forms, so I have a big ask– please test the following XML resources in Specify. You are likely to find issues that aren't related to this PR which I am happy to resolve, so report them. If it grows outside the scope of this, I'll open another and we'll resolve it in the future.

Remember to delete any custom form definitions / app resources before testing this as those will be used first.

  • Create a new DataObjFormatters app resource and verify that all table formats and aggregations appear as expected
  • Build queries and return the (formatted) and (aggregated) results of as many tables as you can, with a focus on new Geo tables. If a table format or aggregation is missing where it should be included, let me know. Some tables (e.g. *property, *attribute, *groupjoin) do not have a table format or aggregation defined as it is ambiguous as to which fields we should display.
  • Create a new TypeSearches app resource and verify that it is working as expected. Delete this resource after it is populated as you should be using the default.
  • Open the CollectionRelType form and search for a collection. You should be able to type the name of a collection and select it. You will encounter HierarchyException when loading Collectionreltype #4989, but that is not in the scope of this issue, and you can safely dismiss it.
  • Create an ExportFeed app resource and verify that you can search for a user in the database by name. You should see the name appear correctly and you should be able to search for a user in the query combo box without the 🔍 QB function.
  • Create a new WebLinks app resource and verify that all web links (~5) appear and work as expected. See this guide for setting up a conditional form on AgentIdentifier to leverage those new web links.
  • Verify that the geology.views.xml can be used as the basis of a new form (create a new form definition and select Geology). Let me know what you think. This is subjective to some extent, and I welcome suggested changes (especially via XML snippets).
  • Test that these new view definitions (mostly shown in this issue's body) are displayed properly (compare to defaults on production):
    • CollectionRelType
      • Verify that you can create a Collection Relationship without the query builder (ignoring this error)
    • Agent should default to Person instead of Organization finally!
      • Agent conditional: If the agent is not a Group, the Members subview should not be shown.
    • TectonicUnit
    • TectonicUnitTreeDef
    • TectonicUnitTreeDefItem
    • TaxonTreeDef
    • Taxon
      • Taxon conditional: Verify that when you check IsHybrid the fields HybridParent1 and HybridParent2 appear. When it is not checked, they should not appear.
    • GeographyTreeDef
    • StroageTreeDef
    • LithoStratTreeDef
    • GeologicTimePeriodTreeDef
    • RelativeAge
    • AbsoluteAge
    • PickList
      • Set the size limit to -1 and verify that you can add any number of pick list items (>500– and you can use the workbench to add the first 500)
    • PickListItem
    • SpAppResource (click pencil next to the title of an app resource)
      • SpAppResource conditional (should show Reports subview button if the resource is a report/label)
        • SpReport form should show ownership, associated query, title, in a read-only view
    • SpViewSetObj (click pencil next to the title of a form definition)
    • SpQuery (click pencil next to a query title)
      • Verify that you can change the ownership of a query
    • RecordSet (click pencil next to a record set title)
      • Verify that you can change the ownership of a record set
    • AgentIdentifier – navigate directly to the form via the data entry menu or by modifying the URL. It should no longer be auto-generated

Tester Comments

TectonicUnit was being rude but I'm pretty sure it's just a database issue that I need to sort on my end. Wasn't able to open it in DataObjFormatters or the Query Builder, and it would give definitions missing errors + couldn't fetch ranks in the form. I can't really pass the first two checks because I can't check that, but I had a few other comments anyway, so I'll retest them both once I've got that figured out. 😅

Reports button missing on the report SpAppResource. It does appear on the label, however.
image

Had an issue with report runner, so I wasn't able to check if it was read only in the labels/reports form. I was able to access it by modifying the link in Data Entry and it wasn't read only there, though.

For the record sets, having owner in the default form means that when you create a record set from query the user has to manually fill in the owner, which is a little annoying, but I assume it's outside the scope of the PR to have it autofill the current user as the owner. I do wonder who is able to transfer ownership to other people, though. There's no specific permissions in place to bar people from transferring right? It would have to be form-based?

I'm unsure about the hard-coding checks, since here on the Picklist form there's a few that look like they might be (Type, Name, Formatter), but that's just going based on my assumption that the field names are always lowercase.

image

That's all I found wrong with the last check, these new default forms are great!

@grantfitzsimmons
Copy link
Member Author

@pashiav Thank you for your review!

I can change ownership, but only through query builder. I cannot select users by searching their name in the field.

No results pop up when I type in a name of a collection. I can only select the collection through query builder.

This is because the database you were testing on has a custom TypeSearches app resource which is used in place of the system default if it is present. Specify doesn't look at the default if it can't find the search definition in the file it already has, unfortunately.

The Reports subview button shows up for Label resources, but does not for Report resources.

Fixed in a2bfb0f (#5367)

All else is expected/OK

@grantfitzsimmons
Copy link
Member Author

@combs-a Thank you for your review!

Reports button missing on the report SpAppResource. It does appear on the label, however.

Fixed in a2bfb0f (#5367)

For the record sets, having owner in the default form means that when you create a record set from query the user has to manually fill in the owner, which is a little annoying, but I assume it's outside the scope of the PR to have it autofill the current user as the owner. I do wonder who is able to transfer ownership to other people, though. There's no specific permissions in place to bar people from transferring right? It would have to be form-based?

Too much hassle for the user– nice catch. I've changed the logic so that the user is set automatically to the current logged-in user when creating a record set: d188aca (#5367)

Everyone with edit access to the record set (and query) tables can transfer ownership at this point. The hope is that this is not abused, and it can be disabled by a simple form edit. I'm not completely satisfied with it but I do not want to expand the scope of this PR too much until we find it to be problematic.

Let me know if you have any trouble with this!

I'm unsure about the hard-coding checks, since here on the Picklist form there's a few that look like they might be (Type, Name, Formatter), but that's just going based on my assumption that the field names are always lowercase.

Removed hard-coded labels here: e10958c (#5367)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 - Forms Issues that are related to the form system
Projects
Status: Dev Attention Needed
7 participants