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

Set default cotype on the frontend #5224

Merged
merged 17 commits into from
Oct 17, 2024
Merged

Set default cotype on the frontend #5224

merged 17 commits into from
Oct 17, 2024

Conversation

sharadsw
Copy link
Contributor

Fixes #5223

Checklist

  • Self-review the PR after opening it to make sure the changes look good
    and self-explanatory (or properly documented)
  • Add automated tests
  • Add relevant issue to release milestone

Testing instructions

  • Go to Data Entry -> Collection Object
  • Verify the default CoType on the form is same as the Collection's CoType

@sharadsw sharadsw added this to the 7.9.7 milestone Aug 14, 2024
@sharadsw
Copy link
Contributor Author

Hmm, this breaks a couple of resource api tests which doesn't sound good

describe('needsSaved', () => {
test('changing field makes needsSaved true', () => {
const resource = new tables.CollectionObject.Resource({
id: collectionObjectId,
});
expect(resource.needsSaved).toBe(false);
resource.set('text1', 'a');
expect(resource.needsSaved).toBe(true);
});
test('changing dependent relationship makes needsSaved true', () => {
const resource = new tables.CollectionObject.Resource({
id: collectionObjectId,
});
expect(resource.needsSaved).toBe(false);
resource.set('determinations', []);
expect(resource.needsSaved).toBe(true);
});
});

@CarolineDenis CarolineDenis self-requested a review August 15, 2024 14:22
@CarolineDenis CarolineDenis modified the milestones: 7.9.7, 7.9.8 Aug 15, 2024
@sharadsw sharadsw changed the base branch from production to issue-5172 August 19, 2024 15:46
@sharadsw sharadsw changed the base branch from issue-5172 to production August 19, 2024 15:46
Copy link
Contributor Author

@sharadsw sharadsw left a comment

Choose a reason for hiding this comment

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

Had to do some workarounds here to get the tests working. Lots of tests involving CO don't have a linked CollectionObjectType or a linked Collection -> CollectionObjectType. Some tests used data from static test files, which I have updated to include Collection/Cotype.

Let me know if there's a better way to do any of these.

@sharadsw sharadsw requested a review from a team September 9, 2024 16:35
Copy link
Collaborator

@emenslin emenslin 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

  • Go to Data Entry -> Collection Object
  • Verify the default CoType on the form is same as the Collection's CoType

This does work but I am a little confused about the query behavior. When querying with cot as the base table it works as I expect showing the 2 cot I made in this collection, but when querying with collection as the base table cot is formatted and only shows the default. Since we added the ability to create multiple cot shouldn't it be aggregated when collection is the base table?

Querying on collection:
image

Querying on cot:
image

@pashiav pashiav requested a review from a team September 10, 2024 16:54
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

  • Go to Data Entry -> Collection Object
  • Verify the default CoType on the form is same as the Collection's CoType

I have the same concerns as #5224 (review), but it works!

@sharadsw
Copy link
Contributor Author

@emenslin That does look strange. I think it's happening because each collection record in the db gets linked to only one collectionObjectType (the default one) and so using collection as the base table only shows the defaults. Not sure if that can be changed unless we change the schema

@grantfitzsimmons
Copy link
Member

@emenslin That does look strange. I think it's happening because each collection record in the db gets linked to only one collectionObjectType (the default one) and so using collection as the base table only shows the defaults. Not sure if that can be changed unless we change the schema

The issue is that the schema caption for the default COT relationship field should clarify this. Currently, it appears ambiguous, suggesting there is only one COT in the collection.

@melton-jason
Copy link
Contributor

@emenslin That does look strange. I think it's happening because each collection record in the db gets linked to only one collectionObjectType (the default one) and so using collection as the base table only shows the defaults. Not sure if that can be changed unless we change the schema

The issue is that the schema caption for the default COT relationship field should clarify this. Currently, it appears ambiguous, suggesting there is only one COT in the collection.

Yep, if I remember correctly the Collection -> CollectionObjectType relationship is only to denote the default collection object type for the Collection.
Multiple CollectionObjectTypes can exist in a single Collection, but there can only be one default.

@sharadsw
Copy link
Contributor Author

sharadsw commented Oct 1, 2024

I agree that the schema caption could be clearer. We can make that change as a part of #5290

Copy link
Contributor

@melton-jason melton-jason left a comment

Choose a reason for hiding this comment

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

This implementation works, but maybe it would be worth changing businessrules a little (in this case specifcially for customInit rules) so we can await businessRuleManager.pendingPromise instead of await new Promise(process.nextTick) in tests.

Also, maybe it'd be worth just including the Collection -> CollectionObjectType in a domain context call so we can synchronously access the collection's default CollectionObjectType wherever the schema has already been fetched.
This would save a lot of calls to the backend, and be significantly faster from the user's perspective.

More information in my comments!

Copy link
Contributor

@melton-jason melton-jason left a comment

Choose a reason for hiding this comment

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

Looks good! Take a look at my recent changes.
Instead of just returning the ID of the CollectionObjectType in the /context/domain.json endpoint, I thought it might be easier if we just directly format it as the the api uri.

I updated the static /context/domain.json endpoint for the frontend tests to include the defaultCollectionObjectType property.
I also realized we weren't using a LOT of the static files in the */tests/fixtures/ directory, so I removed all the files which are unused.

@melton-jason melton-jason requested a review from a team October 12, 2024 18:37
Copy link
Collaborator

@emenslin emenslin 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

  • Go to Data Entry -> Collection Object
  • Verify the default CoType on the form is same as the Collection's CoType

Looks good!

@emenslin emenslin requested a review from a team October 17, 2024 14:54
@sharadsw sharadsw merged commit 319ed17 into production Oct 17, 2024
12 checks passed
@sharadsw sharadsw deleted the issue-5223 branch October 17, 2024 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Set a default CollectionObjectType on the frontend
6 participants