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

Remove unused mark_for_deletion functionality #128

Closed
wants to merge 2 commits into from

Conversation

dougal
Copy link

@dougal dougal commented Nov 1, 2016

Found some unused functionality while spelunking the codebase. Of course this might be used with tools you have not published. Below is what I wrote in the first commit:


  • The functionality provided by this was never used in other modules.
  • Record deletion is always done using Repo.delete directly.
  • No tests exercised this code.
  • Code is almost identical to same pattern shown in Ecto.Changelog docs.
  • Code appears to have been pasted from model-to-model as more were added.

Thanks for open-sourcing, I learnt a lot, including the mark_for_deletion pattern.

* The functionality provided by this was never used in other modules.
* Record deletion is always done using Repo.delete directly.
* No tests exercised this code.
* Code is almost identical to same pattern shown in Ecto.Changelog docs.
* Code appears to have been pasted from model-to-model as more were added.
@dougal
Copy link
Author

dougal commented Nov 1, 2016

s/methods/functions/

@jerodsanto
Copy link
Member

Thanks for contributing! Code spelunking is fun 🔍

I haven't looked at this for awhile, but I'm pretty sure that these are necessary as the related Changelog.Episode will call cast_assoc when its changeset is processed via the admin form. That, in turn, calls changeset on the associated records, which calls mark_for_deletion. If the virtual attribute has been set via the params, the record will be deleted.

You are totally right that I haven't tested this well (there are not many tests in the admin), but you could test manually (or add tests!) by setting up an episode with some hosts/guests/channels/sponsors and then try deleting those associations on your branch. (I don't think it'll work.)

Thanks again, I love how many folks are taking the code for a spin! 💚

@dougal
Copy link
Author

dougal commented Nov 1, 2016

Yes, you are right, these are in fact used. In the templates the :delete virtual field is referenced for use in checkboxes for the deletion of things like guests or hosts from episodes.

If only I'd noticed this I could have submitted some tests instead. Thanks for the reply.

@dougal dougal closed this Nov 1, 2016
@jerodsanto
Copy link
Member

No worries, it is subtle and undocumented.

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