Add soft-deletion to companies, jobs, industries #488

brain-geek · 2019-03-08T19:18:53Z

The first version, feel free to give comments.

brain-geek · 2019-03-11T21:51:46Z

@doomspork @tajchumber @gemantzu @burden Guys, what do you think about this?

In context of #483 I believe having soft-delete will improve traceability a lot.

doomspork · 2019-03-11T22:03:00Z

@brain-geek I will look today! Sorry for the delay. I hope that's okay. What do you say?

brain-geek · 2019-03-11T22:05:43Z

@doomspork Thanks, no worries!

Just trying to push it out before beginning the next thing. 😄

doomspork · 2019-03-11T22:08:15Z

You're the best @brain-geek! 😁

doomspork

Super stuff @brain-geek! Some thoughts and a few changes. Happy to discuss any of them 😁

lib/companies/schema/industry.ex

test/companies/companies_test.exs

test/companies/jobs_test.exs

lib/companies/jobs.ex

lib/companies/companies.ex

lib/companies/schema/company.ex

test/companies/companies_test.exs

test/companies/jobs_test.exs

brain-geek · 2019-03-11T23:08:12Z

@doomspork Thanks for the feedback, I will be applying it in the following days.

Nitrino · 2019-03-12T10:25:48Z

Recently, Devon C. Estes has written the article about soft deletion using the PostgreSQL schema.
http://devonestes.herokuapp.com/soft-delete-with-ecto-3-and-postgres

May use a similar approach, then we will not have to worry about the where(is_deleted: false) conditions.
What do you think about it?

doomspork · 2019-03-12T14:22:13Z

Good point @Nitrino!

When we retweet that article on Elixir School someone had responded suggesting this was not an ideal approach. It might be worth looking into their feedback: https://twitter.com/atomkirk/status/1101566414668869637

brain-geek · 2019-03-12T14:24:59Z

@Nitrino Thanks for the link. I've looked through the article and found it interesting. However, I believe that this project is also intended to be checked out by new Elixir devs, and having some magic inside the postgresql would be hard to follow.

However, it may be better to change is_deleted field to something like removed_pending_change_id to always have a pending change which led to this deletion.

What do you think?

brain-geek · 2019-03-12T14:26:02Z

@doomspork well, I don't agree with like UNIQUE of only live records part in this comment, as you can do unique index with where attached, so it's not a big deal.

doomspork · 2019-03-12T17:00:14Z

However, I believe that this project is also intended to be checked out by new Elixir devs, and having some magic inside the postgresql would be hard to follow.

💯 — very good point. I think this is reason enough not to use it.

However, it may be better to change is_deleted field to something like removed_pending_change_id to always have a pending change which led to this deletion.

Are you suggesting ditching the boolean is_deleted in favor of just checking for the existence of an associated pending change here? I could get behind that 👍

Nitrino · 2019-03-12T17:05:58Z

@brain-geek I agree, the approach with schemas is opaque for new developers.

brain-geek · 2019-03-13T19:18:41Z

@doomspork Please review again. I believe I fixed all the issues. 😄

test/companies/jobs_test.exs

test/support/factories.ex

priv/repo/migrations/20190308180634_soft_deletion.exs

gemantzu · 2019-03-14T08:22:07Z

Can I add my two cents, as I am not totally behind doing this this way right now?

My take on this issue would be to add an active field on companies, jobs (idk about industries, still trying to figure out what we should do with industries) and let pending_changes do it's job here (make the field active or inactive per request).

Also, to add full trackability on what changes we have, we could change pending_changes to have an record id field (we already have the corresponding table name on it), so we can track ( and probably show? ) what changes and on what date have occured on a record.

I am not saying we should not do it like this, I am just stating some thoughts out loud.

doomspork · 2019-03-25T18:52:47Z

@gemantzu / @brain-geek where are we with this? Do we want to move ahead with this approach and come back to @gemantzu's comments or look at incorporating that now?

Happy to help with the work 😁

brain-geek · 2019-03-25T20:14:37Z

@doomspork I'll have comments fixed today.

brain-geek · 2019-03-25T20:30:12Z

Hm, @doomspork , I don't think this approach differs much from one proposed by @gemantzu .

This change only adds a field to mark a record as deleted. Only data attached there is why (pending change reference). My take on this issue would be to add an active field would basically do the same as the current implementation.

Also, if someone deleted a company - that's for a good reason. Let them create a new one instead, IMO.

Having full accountability would be nice, and I agree that would be a useful feature.

doomspork

One small change and this looks good to me @brain-geek. I'd love to get at least one other set of eyes on this before merging though.

cc @tajchumber / @gemantzu / @Nitrino

test/companies/jobs_test.exs

brain-geek · 2019-03-25T23:12:20Z

@doomspork Fixed. Let's wait for other reviews.

lib/companies/jobs.ex

gemantzu · 2019-03-26T18:41:26Z

I see an error when trying to manually test the functionality.
Creating a company and approving it works.
If I make an edit or a delete pending change (both companies and jobs), when I try to view it to either approve or reject, it fails with the following error:
cannot encode association :removed_pending_change from Companies.Schema.Company to JSON because the association was not loaded.
cannot encode association :removed_pending_change from Companies.Schema.Job to JSON because the association was not loaded.

Do you see them in your local repo?

doomspork · 2019-03-29T22:55:16Z

I will test this out this weekend @brain-geek and see if I can reproduce @gemantzu's aforementioned errors 👍

brain-geek · 2019-03-29T23:54:17Z

Sorry for slow responses. I've seen the feedback, will be fixing it this or next weekend.

doomspork · 2019-03-30T00:17:26Z

No worries @brain-geek, take your time. Thanks again for the help! Happy Friday 😁

doomspork · 2019-12-07T23:29:58Z

@maartenvanvliet do you mind taking a peek at this PR? @brain-geek is getting back into open source and wants to wrap up these two outstanding out PRs of his (the other is draft #483). Thank you again for all your help 💜

doomspork · 2019-12-07T23:52:48Z

@brain-geek I fixed the failing tests and the conflict locally. If you give me contributor access to your fork I can push the commit over to ya so we can get this merged 😁

You may also be able to cherry-pick it:
928e858

brain-geek · 2019-12-08T08:32:07Z

@doomspork Wow, thanks a lot! Cherry picked and merged.

maartenvanvliet

A few comments but looks good

lib/companies/jobs.ex

lib/companies/schema/company.ex

lib/companies/schema/industry.ex

lib/companies/schema/job.ex

doomspork · 2019-12-08T16:34:12Z

Thank you so much for taking a look @maartenvanvliet 💜

@brain-geek you're most welcome buddy! Always happy to help ya 😁 ~~Let me know if you want a hand with @maartenvanvliet's comment~~

@brain-geek 6696915

brain-geek · 2019-12-16T19:08:48Z

@doomspork Thanks for help! It feels like cheating. 🔥

brain-geek · 2019-12-16T19:22:07Z

@maartenvanvliet Could you look at this PR once again? I've applied your changes.

@doomspork Thanks for the help! Any ideas what also needs to be changed before merging?

maartenvanvliet

LGTM

doomspork added the enhancement A new feature or enhancement to the existing functionality label Mar 8, 2019

doomspork requested review from doomspork and gemantzu March 8, 2019 19:23

brain-geek force-pushed the soft_deletion branch from 1147955 to 2a15c53 Compare March 8, 2019 19:23

brain-geek mentioned this pull request Mar 11, 2019

Add automatic periodic check of URLs in jobs #483

Closed

doomspork requested changes Mar 11, 2019

View reviewed changes

brain-geek force-pushed the soft_deletion branch from ce27512 to cbf64bc Compare March 13, 2019 19:18

doomspork requested changes Mar 13, 2019

View reviewed changes

test/companies/jobs_test.exs Show resolved Hide resolved

test/support/factories.ex Outdated Show resolved Hide resolved

priv/repo/migrations/20190308180634_soft_deletion.exs Show resolved Hide resolved

priv/repo/migrations/20190308180634_soft_deletion.exs Show resolved Hide resolved

brain-geek force-pushed the soft_deletion branch 2 times, most recently from 6ebe4d2 to 027d3c3 Compare March 25, 2019 20:49

doomspork requested changes Mar 25, 2019

View reviewed changes

test/companies/jobs_test.exs Outdated Show resolved Hide resolved

test/companies/jobs_test.exs Show resolved Hide resolved

brain-geek force-pushed the soft_deletion branch from 027d3c3 to e1fcf64 Compare March 25, 2019 22:52

doomspork reviewed Mar 26, 2019

View reviewed changes

lib/companies/jobs.ex Outdated Show resolved Hide resolved

brain-geek force-pushed the soft_deletion branch from e1fcf64 to 30f0951 Compare November 20, 2019 17:40

brain-geek requested a review from a team as a code owner November 20, 2019 17:40

brain-geek force-pushed the soft_deletion branch from d6d6eaa to 6958222 Compare December 8, 2019 08:31

maartenvanvliet previously approved these changes Dec 8, 2019

View reviewed changes

lib/companies/jobs.ex Outdated Show resolved Hide resolved

lib/companies/jobs.ex Outdated Show resolved Hide resolved

lib/companies/schema/company.ex Show resolved Hide resolved

lib/companies/schema/industry.ex Show resolved Hide resolved

lib/companies/schema/job.ex Show resolved Hide resolved

maartenvanvliet self-requested a review December 8, 2019 22:04

brain-geek dismissed maartenvanvliet’s stale review via 6696915 December 16, 2019 19:09

brain-geek force-pushed the soft_deletion branch 3 times, most recently from 720b8a4 to 1fc361e Compare December 16, 2019 19:21

brain-geek and others added 3 commits December 16, 2019 21:21

Add soft-deletion to companies, jobs, industries

be2522a

update tests

acef488

Filter outs jobs for deleted companies

1fc361e

maartenvanvliet approved these changes Dec 17, 2019

View reviewed changes

doomspork approved these changes Dec 19, 2019

View reviewed changes

doomspork merged commit b9cb801 into beam-community:master Dec 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add soft-deletion to companies, jobs, industries #488

Add soft-deletion to companies, jobs, industries #488

brain-geek commented Mar 8, 2019 •

edited

Loading

brain-geek commented Mar 11, 2019

doomspork commented Mar 11, 2019

brain-geek commented Mar 11, 2019

doomspork commented Mar 11, 2019

doomspork left a comment

brain-geek commented Mar 11, 2019

Nitrino commented Mar 12, 2019

doomspork commented Mar 12, 2019

brain-geek commented Mar 12, 2019

brain-geek commented Mar 12, 2019

doomspork commented Mar 12, 2019

Nitrino commented Mar 12, 2019

brain-geek commented Mar 13, 2019

gemantzu commented Mar 14, 2019 •

edited

Loading

doomspork commented Mar 25, 2019

brain-geek commented Mar 25, 2019

brain-geek commented Mar 25, 2019 •

edited

Loading

doomspork left a comment

brain-geek commented Mar 25, 2019

gemantzu commented Mar 26, 2019

doomspork commented Mar 29, 2019

brain-geek commented Mar 29, 2019

doomspork commented Mar 30, 2019

doomspork commented Dec 7, 2019

doomspork commented Dec 7, 2019 •

edited

Loading

brain-geek commented Dec 8, 2019

maartenvanvliet left a comment

doomspork commented Dec 8, 2019 •

edited

Loading

brain-geek commented Dec 16, 2019

brain-geek commented Dec 16, 2019

maartenvanvliet left a comment

Add soft-deletion to companies, jobs, industries #488

Add soft-deletion to companies, jobs, industries #488

Conversation

brain-geek commented Mar 8, 2019 • edited Loading

brain-geek commented Mar 11, 2019

doomspork commented Mar 11, 2019

brain-geek commented Mar 11, 2019

doomspork commented Mar 11, 2019

doomspork left a comment

Choose a reason for hiding this comment

brain-geek commented Mar 11, 2019

Nitrino commented Mar 12, 2019

doomspork commented Mar 12, 2019

brain-geek commented Mar 12, 2019

brain-geek commented Mar 12, 2019

doomspork commented Mar 12, 2019

Nitrino commented Mar 12, 2019

brain-geek commented Mar 13, 2019

gemantzu commented Mar 14, 2019 • edited Loading

doomspork commented Mar 25, 2019

brain-geek commented Mar 25, 2019

brain-geek commented Mar 25, 2019 • edited Loading

doomspork left a comment

Choose a reason for hiding this comment

brain-geek commented Mar 25, 2019

gemantzu commented Mar 26, 2019

doomspork commented Mar 29, 2019

brain-geek commented Mar 29, 2019

doomspork commented Mar 30, 2019

doomspork commented Dec 7, 2019

doomspork commented Dec 7, 2019 • edited Loading

brain-geek commented Dec 8, 2019

maartenvanvliet left a comment

Choose a reason for hiding this comment

doomspork commented Dec 8, 2019 • edited Loading

brain-geek commented Dec 16, 2019

brain-geek commented Dec 16, 2019

maartenvanvliet left a comment

Choose a reason for hiding this comment

brain-geek commented Mar 8, 2019 •

edited

Loading

gemantzu commented Mar 14, 2019 •

edited

Loading

brain-geek commented Mar 25, 2019 •

edited

Loading

doomspork commented Dec 7, 2019 •

edited

Loading

doomspork commented Dec 8, 2019 •

edited

Loading