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

Add initial state to suggestions #4347

Merged
merged 7 commits into from
Oct 27, 2023

Conversation

allisonking
Copy link
Contributor

@allisonking allisonking commented Oct 26, 2023

Closes https://ethyca.atlassian.net/browse/PROD-1255

Description Of Changes

This was a very strange bug, and I think it was introduced by #4294 (comment) but even that seems to be a bit of a red herring...

First I found that if I reverted that particular change described in that comment, things worked. I did a diff between systemBody and result.data, then added all the fields until I found the one that was causing things to break which was mysteriously third_country_transfers. There's no rhyme or reason to this as far as I can tell, so I dug further...

Then I realized the only fields that were being reset were the dictionary fields, so I looked specifically at useDictSuggestion.

It seems like this line https://github.com/ethyca/fides/blob/aking/prod-1255/fix-system-info-saving/clients/admin-ui/src/features/system/dictionary-form/DictSuggestionInputs.tsx/#L88-L92 was causing problems. The initialValues were properly set, but that bit of code would reset all those fields to empty strings so values would be incorrect. Again, this only happens when third_country_transfers is included, which is very strange, and I suspect is not entirely the root cause and may be a red herring (or a sign of something else).

By adding a new suggestion state initial we can get around this—that line will only fire when we actually want "presuggestions" which I think works pretty well.

Screen.Recording.2023-10-26.at.12.27.21.PM.mov

Code Changes

  • Add a new initial state to suggestions

Steps to Confirm

  • See steps in Roger's bug report.
  • Make sure "pre suggestions" still work

Pre-Merge Checklist

  • All CI Pipelines Succeeded
  • Issue Requirements are Met
  • Update CHANGELOG.md

@allisonking
Copy link
Contributor Author

If we are very lucky, perhaps this will fix #4288 too 🤞

@cypress
Copy link

cypress bot commented Oct 26, 2023

Passing run #4879 ↗︎

0 4 0 0 Flakiness 0
⚠️ You've recorded test results over your free plan limit.
Upgrade your plan to view test results.

Details:

Merge f23be95 into 6460408...
Project: fides Commit: cb64eb5335 ℹ️
Status: Passed Duration: 00:57 💡
Started: Oct 27, 2023 3:18 AM Ended: Oct 27, 2023 3:19 AM

Review all test suite changes for PR #4347 ↗︎

@allisonking allisonking marked this pull request as ready for review October 26, 2023 17:49
@allisonking
Copy link
Contributor Author

allisonking commented Oct 26, 2023

Investigating CI failures which appear to be on main now... I don't believe they are related to this PR, but I will make sure.

Copy link
Contributor

@TheAndrewJackson TheAndrewJackson left a comment

Choose a reason for hiding this comment

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

Thanks for digging into this. It's very weird that that specific field is causing this issue. I'm surprised that this hasn't come up yet already.

The fix itself makes sense though!

:shipit:

}).as("updateParty");
cy.intercept("POST", `/api/v1/plus/custom-metadata/custom-field/bulk`, {
body: {},
}).as("bulkUpdateCustomField");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is a test that needed to be updated after the changes in ba7f66b

]);
}
);
cy.wait("@bulkUpdateCustomField").then((interception) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

also a result of the new bulk update endpoint (which is a lot nicer than what we used to have!)

@allisonking allisonking merged commit be1eac0 into main Oct 27, 2023
@allisonking allisonking deleted the aking/prod-1255/fix-system-info-saving branch October 27, 2023 11:59
allisonking added a commit that referenced this pull request Oct 27, 2023
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