destroy dependent account objects

[tilsammans]

themings and folders must not be orphaned, because they will break the UI in some ways.

I don't know if you like this spec style, I can do it differently, but this is what I usually do.

[houndci-bot]

Avoid using {...} for multi-line blocks.

[tilsammans]

@tf your input is appreciated ... the test fails likely because i added a default theming name. however it's a required field and without a valid theming name default my new feature won't pass test either. It seems to me that default is a fine name for the default theming, especially since it validates against the array set in Pageflow config. Help?

[tf]

The failing test submits an account form without theme_name as an example of invalid attributes. The default value turns that into a valid request.

I checked out your branch and your added tests also passed without this line since the default theming created in the account factory already has a theme name or am I missing something?

Two minor nitpicks:

• Should we use dependent: :delete_all for the folders? There could potentially be many and they are so simple that it might not be worth deleting each in a separate query.
• change(Pageflow::Theming, :count) could be change(Theming, :count) (or maybe even change { Theming.count } - but that's just a matter of taste) since we already are in the Pageflow modules.

EDIT: Feel free to ignore the rest of this comment

There is one bigger issue which I just came to realize: Admins can change entries to use a theming that is associated with a different account. While it has been handy in the past to test new themes without moving entries between accounts, it is something we have to be aware of now when we are deleting themings.

For accounts we prevent deletion as long as entries or users are associated. We could do something similar for themings. That would probably require some hints in the UI.

All other solutions I can think of seem counter intuitive to me:

• Move the used theming to another account?
• Reset the entry's themings to the account's default theme?
• ?

The optimal solution from my point of view would be to display a disabled delete button with some sort of tooltip explaining why the account cannot be removed yet, instead of simply hiding the button the way we do at the moment. What do you think?

Sorry for turning this into a much bigger thing. I'll be happy to help once we've settled on a solution.

[tf]

On second thought, maybe all of this makes things too complicated.

The main use case for changing an entry's theming instead of its account has been to change its theme while still giving the same group of users access. This will become much easier when I start working on allowing users to be members of multiple accounts next week. So as long as there is no real support for multiple themings per account, we should probably simply remove the theming drop down from the entry form.

So for the sake of this PR, we can simply assume that there are no more entries using an account's themings once the account is deleted. That way there will only be errors if you configured entries in special way instead of every time you delete an account.

Sorry for the wall of text.

[tilsammans]

Should we use dependent: :delete_all for the folders?

I feel safer with :destroy since that will fire callbacks.

change(Pageflow::Theming, :count) could be change(Theming, :count)

Updated!

With regards to the rest of your comment, I agree it should be done in principle but if you're already planning on working this in the short term, I suppose it's a lot of work for not a lot of gain. And then keeping the scope of this PR small makes me happier; especially since it will plug a known usability issue.

PR updated.

[tf]

Agreed! Please squash the commits and we're good to merge.

[tf]

Could you take a second look at the commit message. It still talks about default theme naming etc...

[tilsammans]

@tf all done

[tf]

👍