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

Delete scripts #188

Merged
merged 16 commits into from
Sep 10, 2019
Merged

Delete scripts #188

merged 16 commits into from
Sep 10, 2019

Conversation

squidsoup
Copy link
Contributor

@squidsoup squidsoup commented Sep 8, 2019

Note

This is dependent on #183. Leaving this on WIP until 183's merged, to make this easier to review.

Done

  • Implement deleting scripts.

QA

  • Visit testing/commissioning scripts.
  • Ensure default scripts (those provided by MAAS) cannot be deleted.
  • Upload a testing or commissioning script via old maas, revisit the testing/commissioning page in maas-ui to update the list.
  • Delete the script, you should see a confirmation and the script should disappear from the list.

Copy link
Contributor

@huwshimi huwshimi left a comment

Choose a reason for hiding this comment

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

Probably worth adding some error handling e.g. if you remove a script from the old UI and then try to remove the same one from the new ui.

ui/src/app/settings/views/Scripts/ScriptsList.js Outdated Show resolved Hide resolved
@squidsoup
Copy link
Contributor Author

squidsoup commented Sep 10, 2019

Now displays errors e.g.
image

@huwshimi
Copy link
Contributor

Strangely this is what I now get when deleting a script:

Screen Shot 2019-09-10 at 2 41 13 pm

@lyubomir-popov
Copy link
Contributor

After deleting the scripts, we see 0:
image
Can that turn to a #666 no scripts available message? @lilyvidenova I think we made a design for this somewhere recently?

@lyubomir-popov
Copy link
Contributor

Would it be too much to patch the side nav on small screens as a drive by?
All it would take is:

@media (max-width: $breakpoint-medium) {
  .settings-nav__list {
      list-style: none;
      columns: 2;
      margin-left: 0;
      padding-left: 0;
      margin-bottom: 0.5rem;
      margin-top: -0.25rem;
  }
  .settings-nav__list .settings-nav__list .settings-nav__item + .settings-nav__item {
    border-top: 1px solid #cdcdcd;
  }
}

image

@lilyvidenova
Copy link

Hmm I'm getting lots of error notifications:

Screenshot 2019-09-10 at 11 56 39

@squidsoup
Copy link
Contributor Author

After deleting the scripts, we see 0:
image
Can that turn to a #666 no scripts available message? @lilyvidenova I think we made a design for this somewhere recently?

We have a card for this one #184

@squidsoup
Copy link
Contributor Author

squidsoup commented Sep 10, 2019

Hmm I'm getting lots of error notifications:

Screenshot 2019-09-10 at 11 56 39

@lilyvidenova that's not an error I've seen. A 504 would suggest that the MAAS server can't be reached.

Do you have REACT_APP_BASE_URL configured in your .env.development.local? Presumably you can see the list of scripts? I'm going to land this branch, but please open an issue with steps to reproduce this when you have a moment.

@squidsoup squidsoup merged commit d2d065e into canonical:master Sep 10, 2019
@squidsoup squidsoup deleted the delete-scripts branch September 10, 2019 22:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants