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

ERM-3392 Changes to the field content type are not saved, but a success message is displayed #1368

Merged
merged 5 commits into from
Nov 13, 2024

Conversation

CalamityC
Copy link
Contributor

@CalamityC CalamityC commented Nov 11, 2024

  • change agreementContentTypes initialValues on submit

…ss message is displayed

* change agreementContentTypes initialValues on submit
Copy link

License CLA Stuck? (Developer should make sure that it is really stuck before clicking)

Copy link

github-actions bot commented Nov 11, 2024

Jest Unit Test Results

    1 files  ±0    156 suites  ±0   4m 14s ⏱️ +16s
1 048 tests ±0  1 048 ✅ ±0  0 💤 ±0  0 ❌ ±0 
1 055 runs  ±0  1 055 ✅ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit b90636c. ± Comparison against base commit e91e1b1.

♻️ This comment has been updated with latest results.

@EthanFreestone EthanFreestone changed the title ERM-3392 Changes to the field content type are not saved, but a succe… ERM-3392 Changes to the field content type are not saved, but a success message is displayed Nov 11, 2024
Copy link
Contributor

@EthanFreestone EthanFreestone left a comment

Choose a reason for hiding this comment

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

This isn't right. This will disregard any _delete instructions.

We should be able to instead have the actual form manipulate the id instead, so there's nofanagling needed on submit.

* fix field name, add parse and format
@CalamityC CalamityC marked this pull request as ready for review November 12, 2024 14:20
… of field level stuff

In general, Claudia's solution is "nicer" in that it prevents ANY incorrect information from appearing in the PUT.

However, it ends up doing a bunch of extra work inside the Select which we would then need to replicate everywhere to get the "best" solution consistently.

We could (and an argument exists should) do this in a RefdataSelect type component, but then that would require special testing etc etc, and to be used in a very specific way. The easier fix (which is already used in some other places) is simply to use the id _only_.

This makes some of the "only display refdata in use" logic a bit more complex, so we maybe ought to improve that and centralise it as well.

ERM-3392
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
55.6% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

@EthanFreestone EthanFreestone merged commit 782ed41 into master Nov 13, 2024
17 of 18 checks passed
@EthanFreestone EthanFreestone deleted the erm-3392 branch November 13, 2024 15:05
Jack-Golding pushed a commit that referenced this pull request Nov 29, 2024
…ss message is displayed (#1368)

* ERM-3392 Changes to the field content type are not saved, but a success message is displayed

* change agreementContentTypes initialValues on submit

* * revert submit handler changes
* fix field name, add parse and format

* fix button

* chore: Change over to just "id" in select to avoid having to do loads of field level stuff

In general, Claudia's solution is "nicer" in that it prevents ANY incorrect information from appearing in the PUT.

However, it ends up doing a bunch of extra work inside the Select which we would then need to replicate everywhere to get the "best" solution consistently.

We could (and an argument exists should) do this in a RefdataSelect type component, but then that would require special testing etc etc, and to be used in a very specific way. The easier fix (which is already used in some other places) is simply to use the id _only_.

This makes some of the "only display refdata in use" logic a bit more complex, so we maybe ought to improve that and centralise it as well.

ERM-3392

---------

Co-authored-by: EthanFreestone <[email protected]>
Co-authored-by: Ethan Freestone <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants