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

Allow user to delete the scenarios agents with it #1446

Merged
merged 1 commit into from
Apr 24, 2016

Conversation

dsander
Copy link
Collaborator

@dsander dsander commented Apr 21, 2016

The scenario delete button now opens a modal in which the user can choose if and how the scenarios agents should be deleted with it.

screenshot 2016-04-21 13 01 51

Thanks to kreuzwerker.de who are paying me to work on this in the context of the research project 'Digitale Kuratierungstechnologien' of which they are a part of.

expect(unique_agent_ids).to eq([agents(:jane_rain_notifier_agent).id])
end

it "returns not agents when all are also used in a different scenario" do
Copy link
Member

Choose a reason for hiding this comment

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

not == no?

@cantino
Copy link
Member

cantino commented Apr 22, 2016

Good idea!

<div class="modal-header">
<button type="button" class="close" data-dismiss="modal"><span aria-hidden="true">&times;</span><span class="sr-only">Close</span></button>
<h4 class="modal-title">
<% if scenario == @scenario %>
Copy link
Member

Choose a reason for hiding this comment

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

I think this works because in once case @scenario is nil? Might be clearer as if @scenario && scenario == @scenario

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

True, it works because we only assign @scneario in the show page.

Copy link
Member

Choose a reason for hiding this comment

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

Both are technically fine. I just thought testing @scenario might be clearer for readability. I don't feel strongly.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see you made the change 😄

@cantino
Copy link
Member

cantino commented Apr 23, 2016

Looks great!

@dsander
Copy link
Collaborator Author

dsander commented Apr 24, 2016

Thanks!

@dsander dsander merged commit ea95a9d into huginn:master Apr 24, 2016
@dsander dsander deleted the feature/scenario-deletion branch April 24, 2016 08:28
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.

2 participants