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 campaign #762

Merged
merged 24 commits into from
Nov 27, 2019
Merged

Delete campaign #762

merged 24 commits into from
Nov 27, 2019

Conversation

Eleonore9
Copy link
Collaborator

@Eleonore9 Eleonore9 commented Nov 7, 2019

  • add functionality to delete campaign files on S3
  • implement deletion in users (managers and watchers) json [waiting on campaign json update]
  • implement deletion from the UI and redirect to user's page (My projects)
  • notify the user that the deletion has worked

Changes after review:

  • add text in the popup window "This operation cannot be undone"
  • fix display attribute of popup so it doesn't break in Chrome
  • use bootstrap Modal for the popup window
  • when looking up managers/viewers take into account the new json structure
"campaign_viewers": [
  { osm_id: "123456", name: "Jo Sprague" },
  { osm_id: "987654", name: "Russ Biggs" }
]
  • create a test campaign.json file to test

Closes issue #669

delete-step1
delete-step2
delete-step3

@Eleonore9 Eleonore9 force-pushed the delete-campaign branch 2 times, most recently from ab7bd03 to b13737c Compare November 12, 2019 09:39
@Eleonore9 Eleonore9 changed the title [WIP] Delete campaign Delete campaign Nov 13, 2019
@Eleonore9 Eleonore9 marked this pull request as ready for review November 13, 2019 01:08
@russbiggs
Copy link
Contributor

Looks like there is some styling issues on this:

Screen Shot 2019-11-12 at 8 36 09 PM

@Eleonore9
Copy link
Collaborator Author

@russbiggs this looks more like you can't see half of it! I've added screenshot in the PR description.
I'm not sure of what's going on? How are you running it locally?

const popup = document.getElementById("delete-pop");
const background = document.getElementById("delete-background");
if (popup.style.display == "none" && background.style.display == "none") {
popup.style.display = "inline-table";
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like the screenshot @russbiggs posted was because of this line. Would setting display: block work?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thank you!! Yes, It'll work (with more styling)!

<button class="btn btn-primary btn-xs">

<div id="delete-background" style="display:none;"></div>
<div id="delete-pop" style="display:none;">
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can use the Bootstrap modal here, since the project already has the CSS and JS for it. Here's an example: https://getbootstrap.com/docs/3.4/javascript/#modals-examples

We're currently using this for other modals in the app, like the warning when someone tries to click "Create" before logging in, and the modal when a user adds types during campaign creation.

We'll have to write some CSS overrides to make the modal match the new design. I think those overrides could go in flask_project/campaign_manager/styles/components/_modal.scss.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK, thanks! I'll change to using a modal for the popup! 👌

@russbiggs
Copy link
Contributor

russbiggs commented Nov 25, 2019

This looks pretty much complete. We just need to hide the delete button in the jinja template in the header in the case where the user is not a manager of the campaign. Although not necessarily secure it would be good to do a check on the delete handler server side as well.

@Eleonore9
Copy link
Collaborator Author

This looks pretty much complete. We just need to hide the delete button in the jinja template in the header in the case where the user is not a manager of the campaign. Although not necessarily secure it would be good to do a check on the delete handler server side as well.

Sure! I hadn't realised it was to be done on my end.

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.

3 participants