Skip to content

Implement saving only one model at a time#2588

Merged
robertbrignull merged 1 commit intomainfrom
robertbrignull/data-save-single-model
Jul 7, 2023
Merged

Implement saving only one model at a time#2588
robertbrignull merged 1 commit intomainfrom
robertbrignull/data-save-single-model

Conversation

@robertbrignull
Copy link
Copy Markdown
Contributor

Paired with @shati-patel on this.

Hooks up the "Save" button in each collapsible section to save only the changes made to that model. Thankfully this turned out not to be too hard since the implementation of saveModeledMethods already (perhaps unintentionally) handles being gives only the methods from one model, because it splits the methods it's given into models and then saves only those files. Therefore it won't overwrite or delete other models that aren't being saved.

We also had to change how setUnsavedModels is called. I think what we were doing before was not quite fully correct but we were getting away with it. Now that we can save just one model at a time we need to be more granular.

We've tested this manually but checking that each model is saved individually and none of the other files are edited. Is there any more testing we can do of this?

Checklist

  • CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
  • Issues have been created for any UI or other user-facing changes made by this pull request.
  • [Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

@robertbrignull robertbrignull requested a review from a team as a code owner July 6, 2023 15:38
Copy link
Copy Markdown
Contributor

@charisk charisk left a comment

Choose a reason for hiding this comment

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

LGTM! Are there any React/unit tests we can add around this area?

@robertbrignull
Copy link
Copy Markdown
Contributor Author

Are there any React/unit tests we can add around this area?

I think the answer is that right now, no not really. It's kinda hard.

There are currently no tests of the data-extensions-editor react code, but we could add some fairly easily. However I don't think testing this particular UI change it too useful. Adding tests of other existing functionality would be useful, but that's outside the scope of this PR.

There are also no tests of DataExtensionsEditorView, and adding some looks hard. I think we want to move implementation out of this class and into separate classes that'll be more testable, but I'm hesitant to do that in this PR. Would it be ok if I open an another issue to cover moving all of the implementation for loading/saving modelled data to a separate file and testing it?

@charisk
Copy link
Copy Markdown
Contributor

charisk commented Jul 7, 2023

Are there any React/unit tests we can add around this area?

I think the answer is that right now, no not really. It's kinda hard.

There are currently no tests of the data-extensions-editor react code, but we could add some fairly easily. However I don't think testing this particular UI change it too useful. Adding tests of other existing functionality would be useful, but that's outside the scope of this PR.

There are also no tests of DataExtensionsEditorView, and adding some looks hard. I think we want to move implementation out of this class and into separate classes that'll be more testable, but I'm hesitant to do that in this PR. Would it be ok if I open an another issue to cover moving all of the implementation for loading/saving modelled data to a separate file and testing it?

Yeah that makes sense! Happy to improve on test coverage separately.

@robertbrignull robertbrignull merged commit dae74e8 into main Jul 7, 2023
@robertbrignull robertbrignull deleted the robertbrignull/data-save-single-model branch July 7, 2023 08:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants