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

Adds dependent restricted association to some models #567

Merged
merged 1 commit into from Dec 18, 2014

Conversation

Projects
None yet
4 participants
@ChrisBr
Copy link
Member

ChrisBr commented Dec 11, 2014

Should fix #348

@ChrisBr ChrisBr added the in progress label Dec 11, 2014

@ChrisBr

This comment has been minimized.

Copy link
Member

ChrisBr commented Dec 11, 2014

@coveralls

This comment has been minimized.

Copy link

coveralls commented Dec 11, 2014

Coverage Status

Coverage decreased (-0.06%) when pulling 0f8f09f on 348 into dab0d9d on master.

@differentreality

This comment has been minimized.

Copy link
Contributor

differentreality commented Dec 13, 2014

We can also add abilities and disable the delete button accordingly.
For instance:
Define the ability:
cannot :destroy, EventType do |type|
Event.where(event_type_id: type.id).present?
end

and in index, add the following in the delete button:
disabled: !(can? :destroy, event_type)

BTW https://github.com/openSUSE/osem/blob/master/app/views/admin/campaigns/_form.html.haml#L22
should be = f.input :targets, collection: @conference.targets

@differentreality

This comment has been minimized.

@differentreality

This comment has been minimized.

Copy link
Contributor

differentreality commented Dec 13, 2014

And I just realized that we have target belongs_to campaign, which messes up everything when a second campaign is created and selects a target which is already used by another campaign too.

I guess we should either change the relationship or allow for a campaign to use only unused targets?

@differentreality

This comment has been minimized.

Copy link
Contributor

differentreality commented Dec 13, 2014

In https://github.com/openSUSE/osem/blob/master/app/controllers/admin/tracks_controller.rb#L48
and
https://github.com/openSUSE/osem/blob/master/app/controllers/admin/rooms_controller.rb#L44
error doesn't work.
http://stackoverflow.com/questions/7510418/rails-redirect-to-with-error-but-flasherror-empty

We should probably decide on a common layout for errors from now on.
I think we have changed most of the controllers to
flash[:error] = ...
redirect_to ...

@ChrisBr

This comment has been minimized.

Copy link
Member

ChrisBr commented Dec 14, 2014

@differentreality Good idea with the disabled button, added it!

Changed the error/notice layout to flash[:error] because with redirect it happens that it doesn't work...

And yeah, something seems messy with campaign / target! But we should fix that in another PR. Furthermore we should rename target to goal because I think that's a better name for (and we already replaced target in the menu). I will examine if there is a handy method to rename model/controller/view

@differentreality @hennevogel Please review my fixes

@coveralls

This comment has been minimized.

Copy link

coveralls commented Dec 14, 2014

Coverage Status

Coverage decreased (-0.09%) when pulling 08f5823 on 348 into dab0d9d on master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Dec 14, 2014

Coverage Status

Coverage decreased (-0.13%) when pulling be9e49a on 348 into dab0d9d on master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Dec 14, 2014

Coverage Status

Coverage decreased (-0.09%) when pulling 1c7440f on 348 into dab0d9d on master.

@ChrisBr

This comment has been minimized.

Copy link
Member

ChrisBr commented Dec 15, 2014

@hennevogel @differentreality

The following dependent are now implemented:

Event -> Track nullify
Event -> Difficulty Level nullify
Event -> Room nullify
Target -> Campaign nullify

Ticket -> Ticket_Purchases destroy

Event -> Event Type restrict_with_error
Actually we assume that an event has an event type at any time, that means that event.event_type is called many times in the whole app. Therefore I would stick to restrict_with_error till we have decided how we will fix that....

@coveralls

This comment has been minimized.

Copy link

coveralls commented Dec 15, 2014

Coverage Status

Coverage decreased (-0.17%) when pulling 80a0ecb on 348 into dab0d9d on master.

@differentreality

This comment has been minimized.

Copy link
Contributor

differentreality commented Dec 16, 2014

I think that for dependent: :nullify we should inform the user that the item is already in use will be deleted (and the events using it will be left without without a track or...).

@ChrisBr

This comment has been minimized.

Copy link
Member

ChrisBr commented Dec 16, 2014

@differentreality Yeah, what do you think about an general information in the delete alert() or do you want an additional information only if it's in use?

Do you have any suggestion for a nice information text?

@coveralls

This comment has been minimized.

Copy link

coveralls commented Dec 17, 2014

Coverage Status

Coverage decreased (-0.17%) when pulling 81e92c5 on 348 into 87b1aba on master.

@@ -100,6 +100,10 @@ def user_with_roles(user)
can :manage, Venue, conference_id: conf_ids_for_organizer
can :index, Venue, conference_id: conf_ids_for_organizer + conf_ids_for_cfp
can :manage, :all if user.is_admin

cannot :destroy, EventType do |type|

This comment has been minimized.

@hennevogel

This comment has been minimized.

@differentreality

differentreality Dec 18, 2014

Contributor

We don't allow used eventtypes to be deleted.
And we disable the delete button too.
https://github.com/openSUSE/osem/blob/348/app/views/admin/event_types/index.html.haml#L35

@ChrisBr

This comment has been minimized.

Copy link
Member

ChrisBr commented Dec 18, 2014

@hennevogel Removed the disabled and ability as discussed! Plese review

@coveralls

This comment has been minimized.

Copy link

coveralls commented Dec 18, 2014

Coverage Status

Coverage decreased (-0.18%) when pulling 799fd0c on 348 into 01c57d0 on master.

@hennevogel

This comment has been minimized.

Copy link
Member

hennevogel commented Dec 18, 2014

👍 then

ChrisBr added a commit that referenced this pull request Dec 18, 2014

Merge pull request #567 from openSUSE/348
Adds dependent restricted association to some models

@ChrisBr ChrisBr merged commit cc80fb1 into master Dec 18, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@ChrisBr ChrisBr removed the in progress label Dec 18, 2014

@ChrisBr ChrisBr deleted the 348 branch Dec 18, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment