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

ui: Topology intention saving improvements #9513

Merged
merged 9 commits into from Jan 19, 2021

Conversation

johncowen
Copy link
Contributor

@johncowen johncowen commented Jan 6, 2021

This PR adds various things:

Save existing intention

Previously (in 1.9.0) if an intention already existed for an upstream, we would link to the intention save form in order for you to save it. Once saved this would push you back to the intention listing page, rather than back to the topology page. The user flow is much nicer when an upstream doesn't have an existing intention, in which case we simply created/saved the intention in-place within the topology view.

In 1.9.1 we accidentally updated things to not distinguish different flows between an upstream with an existing intention and a non-existent intention. This meant that an intention description or metadata could potentially be overwritten if saved from the topology view page.

This PR adds the user flow we actually want, i.e. there is no difference between an upstream with an existing intention and a non-existent intention - we always keep you in the topology view. But crucially here we also check for and use an intention to save if one already exists for the upstream, if not we create a new one for you. I actually wanted to do this slightly differently using a <DataSource /> component instead of the in-route data.source, but that didn't quite work out.

Notifications on saving/failing to save an intention from the topology view

We also add our standard notifications for performing the above actions from within the topology view, which is also a bit of a long story!

From the beginning of time (i.e. back in Ember 2 days) we've used an ember Mixin to reuse our notification functionality across the app. We are moving away from Mixins as they are now deprecated.

A while back we made a <DataForm /> which we can use to save full forms in a composable manner that includes auto notifications for saving and erroring. Whilst this avoids Mixins, its currently still very heavyweight and its innards aren't really split enough as yet - but it does still use our 'from the beginning of time' feedback service.

I wanted to do something with a smaller scope than <DataForm /> for our notifications to help us move away from Mixins even further. I kinda of reluctantly took a look at adding a decorator (I'm not super keen on writing custom decorators as yet), as our old implementation was basically wrapping a function in a try/catch in order to notify, which kinda feels the same as a decorator, but I realised that as decorators wrap an entire method, you can use them to wrap functions that you use within the method:

@notify
@action 
async save() {
  // I only want this line wrapped in try/catch
  await this.repo.persist()
  // not this line
  this.refresh()
}

which led me to just splitting up our notification service so we can use it differently if we need to:

@action
async save() {
  const notification= this.feedback.notification(type)
  try {
   // I only want this line wrapped in try/catch
    let thing = await this.repo.persist()
    notification.success(thing)
  } catch(e) {
   notification.error(e)
  }
  // not this line
  this.refresh()
}

The old style execute still works as before:

this.feedback.execute(() => this.repo.persist())

and both interfaces use the same innards.

Testing

I added some basic tests around this, but purposefully didn't use page-objects. The more I use them the more I wonder if we could do page-objects differently, and more and more as we add more nested components. I played with a different approach but ended up going down a bit of a rabbit hole with it all and stopped before things got too weird.

johncowen and others added 8 commits January 6, 2021 11:34
Previously we risked overwriting existing data in an intention if we
tried to save an intention without having loaded it first, for example
Description and Metadata would have been overwritten.

This change loads in all the intentions for an origin service so we can
pick off the one we need to save and change to ensure that we don't
overwrite any existing data.
..between various interfaces to use it.
specifically without getting involved in any page-objects for the moment
@johncowen johncowen added theme/ui Anything related to the UI backport/1.9 labels Jan 6, 2021
@johncowen johncowen requested a review from kaxcode January 6, 2021 16:58
Copy link
Contributor

@kaxcode kaxcode left a comment

Choose a reason for hiding this comment

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

LGTM

@vercel
Copy link

vercel bot commented Jan 19, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/hashicorp/consul/kkzo9wmwm
✅ Preview: Canceled

@johncowen johncowen merged commit be69436 into master Jan 19, 2021
@johncowen johncowen deleted the ui/feature/topo-save-existing-intention branch January 19, 2021 15:40
@hashicorp-ci
Copy link
Contributor

🍒 If backport labels were added before merging, cherry-picking will start automatically.

To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/311927.

@hashicorp-ci
Copy link
Contributor

🍒✅ Cherry pick of commit be69436 onto release/1.9.x succeeded!

hashicorp-ci pushed a commit that referenced this pull request Jan 19, 2021
* ui: Keep track of existing intentions and use those to save changes

Previously we risked overwriting existing data in an intention if we
tried to save an intention without having loaded it first, for example
Description and Metadata would have been overwritten.

This change loads in all the intentions for an origin service so we can
pick off the one we need to save and change to ensure that we don't
overwrite any existing data.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme/ui Anything related to the UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants