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

11044 edit facet bug #11052

Merged
merged 9 commits into from
Dec 4, 2024
Merged

11044 edit facet bug #11052

merged 9 commits into from
Dec 4, 2024

Conversation

sekmiller
Copy link
Contributor

@sekmiller sekmiller commented Nov 26, 2024

What this PR does / why we need it:
Fixes an issue where the facets would be added multiple times on Edit Dataverse

Which issue(s) this PR closes:

Special notes for your reviewer:
The issue was that while the existing facets were deleted in the DB, they were left as part of the Dataverse being edited so when the dataverse is saved the facets were re-created. Similar situation for field input levels.

Suggestions on how to test this: Edit Dataverse via api and UI and verify that the facets and dataset field input levels are being saved correctly with no doubling of entries. (see original ticket for example of failure state.)

Does this PR introduce a user interface change? If mockups are available, please link/include them here:

Is there a release notes update needed for this change?: No

Additional documentation: None - fixing documented behavior

@sekmiller sekmiller added FY25 Sprint 11 FY25 Sprint 11 (2024-11-20 - 2024-12-04) Size: 3 A percentage of a sprint. 2.1 hours. labels Nov 26, 2024
@sekmiller sekmiller self-assigned this Nov 26, 2024
@qqmyers
Copy link
Member

qqmyers commented Nov 26, 2024

Haven't tried to review - would it possible to also add a db constraint to avoid more than one entry per datasetfieldtype_id + dataverse_id combo?

@coveralls
Copy link

coveralls commented Nov 26, 2024

Coverage Status

coverage: 22.573% (+0.004%) from 22.569%
when pulling ca95ad8 on 11044-edit-facet-bug
into 1b5a1ea on develop.

This comment has been minimized.



}

Choose a reason for hiding this comment

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

🚫 [reviewdog] <com.puppycrawl.tools.checkstyle.checks.whitespace.FileTabCharacterCheck> reported by reviewdog 🐶
File contains tab characters (this is the first instance).

@sekmiller sekmiller removed their assignment Nov 26, 2024
@sekmiller
Copy link
Contributor Author

@qqmyers I thought about adding a DB constraint, but I was worried that if it was violated it would fail badly - or at least in a way that would be difficult for the end user to understand/fix.

This comment has been minimized.

@sekmiller sekmiller added this to the 6.5 milestone Nov 26, 2024

This comment has been minimized.

Copy link
Member

@qqmyers qqmyers 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 - I haven't tested but the changes seem reasonable given the issue. @sekmiller - there are two failing tests though. Perhaps the deleteFile one is just timing but the other looks like it's facet-related. I'll approve, conditional on it being able to pass the tests before QA completes.

em.persist(dataverseFacet);
ownerDv.getDataverseFacets().add(dataverseFacet);
Copy link
Member

Choose a reason for hiding this comment

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

Just curious - did this have to move after the persist?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe not. I put it back and it still seems to work

@ofahimIQSS ofahimIQSS self-assigned this Dec 3, 2024
@ofahimIQSS
Copy link
Contributor

@sekmiller I noticed that "continuous-integration/jenkins/pr-merge" is failing: Ansible run terminated abnormally, failing build.

This comment has been minimized.

@sekmiller sekmiller assigned ofahimIQSS and unassigned sekmiller Dec 3, 2024
Copy link

github-actions bot commented Dec 3, 2024

📦 Pushed preview images as

ghcr.io/gdcc/dataverse:11044-edit-facet-bug
ghcr.io/gdcc/configbaker:11044-edit-facet-bug

🚢 See on GHCR. Use by referencing with full name as printed above, mind the registry name.

@ofahimIQSS
Copy link
Contributor

Passed testing - Merging PR
Testing of 11052.docx

@ofahimIQSS ofahimIQSS merged commit a36db2d into develop Dec 4, 2024
19 checks passed
@ofahimIQSS ofahimIQSS deleted the 11044-edit-facet-bug branch December 4, 2024 15:04
@ofahimIQSS ofahimIQSS removed their assignment Dec 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FY25 Sprint 11 FY25 Sprint 11 (2024-11-20 - 2024-12-04) Size: 3 A percentage of a sprint. 2.1 hours.
Projects
Status: Done 🧹
Development

Successfully merging this pull request may close these issues.

Multiple listing for facets after editing the list
4 participants