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

Append an empty item when editing an existing Secure Variable #13436

Merged
merged 3 commits into from
Jun 20, 2022

Conversation

philrenaud
Copy link
Contributor

Resolves #13394

Appends an empty row to the end of your Items array when you modify an existing Secure Variable.

This ensures that all previous rows are deletable, and with #13424, will compact/remove empty items on save.

@github-actions
Copy link

github-actions bot commented Jun 20, 2022

Ember Asset Size action

As of 10110d3

Files that got Bigger 🚨:

File raw gzip
nomad-ui.js +290 B +76 B

Files that stayed the same size 🤷‍:

File raw gzip
vendor.js 0 B 0 B
nomad-ui.css 0 B 0 B
vendor.css 0 B 0 B

@philrenaud philrenaud requested a review from juliezzhou June 20, 2022 17:29
@github-actions
Copy link

github-actions bot commented Jun 20, 2022

Ember Test Audit comparison

secure-variables 10110d3 change
passes 1321 1321 0
failures 2 2 0
flaky 0 0 0
duration 000ms 000ms -000ms

Copy link
Contributor

@ChaiWithJai ChaiWithJai left a comment

Choose a reason for hiding this comment

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

I'm noticing some lurking errors and warning messages as a result of the save logic. I have a feeling that setAndTrimPath shouldn't be running when the form is in edit mode. We disable the input field on the client, however, we still run setAndTrimPath which resets the id of the model we're currently working on.

This can lead to some nasty side-effects and I get this warning:

WARNING: The 'id' for a RecordIdentifier should not be updated once it has been set. Attempted to set id for 'variable:blue/berkshire/lime (@ember-data:lid-variable-blue/berkshire/lime)' to 'null'.

Now that we're adding a callback that's fired when the form element is appended onto the DOM, we should maybe consider explicitly modeling state.

@@ -1,4 +1,6 @@
<form class="new-secure-variables" autocomplete="off" {{on "submit" this.save}}>
<form class="new-secure-variables" autocomplete="off" {{on "submit" this.save}}
{{did-insert this.appendItemIfEditing}}
Copy link
Contributor

Choose a reason for hiding this comment

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

blocking: The challenge with this logic is that the newly appended row can't be deleted.

https://app.replay.io/recording/secure-variables-blueberkshirelime-americas-nomad--ede01a65-5d04-4e4b-b540-513ff68ce38b

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's correct; but all empty rows are removed on save.

The point of this PR is so that the "can't be deleted" row is a new empty-by-default one, rather than the last one in the list. cc @juliezzhou

@philrenaud philrenaud merged commit 812e94e into secure-variables Jun 20, 2022
@philrenaud philrenaud deleted the 13394-sv-append-temporary-row-on-edit branch June 20, 2022 20:39
tgross pushed a commit that referenced this pull request Jul 11, 2022
* Did-insert modifier to add an extra row when editing

* Defensive logic on model existing

* Defensive pattern on copy keyValues
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Secure Variables: When rendering /edit, append a blank Items[] key/value so the other last row is deletable
2 participants