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

Feature/litigation soft delete #62

Merged
merged 6 commits into from Sep 19, 2019
Merged

Conversation

agwozdowski
Copy link
Contributor

@agwozdowski agwozdowski commented Sep 18, 2019

  1. Added discarded_at to Litigation model
  2. Override DESTROY action using ::Command::Destroy::Litigation
  3. Added specs
  4. Removed validation during soft-delete action in Litigation and also from Legislation because it's possible that some precedent record (like geography) was removed earlier.

@tsubik
Copy link
Collaborator

tsubik commented Sep 18, 2019

I'm on it. 👀

Copy link
Collaborator

@tsubik tsubik left a comment

Choose a reason for hiding this comment

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

Looks good 👍

Just found one error, could be also in other places where we soft-delete. After delete I was redirected to page with scope All in the header bar scope=All, but I think that should be scope=all or no scope param, because I was on the default page. Our default all scope should not show archived records.

@agwozdowski
Copy link
Contributor Author

Thanks @tsubik ! I'll take a look and fix in all places

@agwozdowski
Copy link
Contributor Author

@kowal @tsubik
I made a research with this redirect. It doesn't work on all soft-delete pages.
Solution with current_scope which was used at the beginning will not work because current_scope is always all in this case.

I tried redirect_to request.referer which works properly when user delete it from the list, but it doesn't work from view page when user click DELETE.

We have 2 options:

  1. The easiest - always redirect to all scope
  2. Redirect to last used scope. This solution is more complicated because by default DELETE in active admin doesn't send last scope. It calls http://localhost:3000/admin/geographies/21 with param _method: delete. It's probably doable, but I'm not sure if it's necessary. Before I'll start investigation how override this action want to be sure if want to invest time for it.

@tsubik
Copy link
Collaborator

tsubik commented Sep 18, 2019

Hmm, I think default ActiveAdmin destroy must not have this issue. Maybe it's worth to check the AA code how they redirecting after destroy.

@agwozdowski
Copy link
Contributor Author

Hey @tsubik, @kowal - it's again me :)
I've just checked that in "normal" Destroy - without our overwrite - active admin redirect to the list without any scope.
My suggestion is to implement it similar in our case in all lists.
What do you think?

@kowal
Copy link
Contributor

kowal commented Sep 19, 2019

Fine for me, let's not block this story (since the problem was there before). We can improve those redirect later. @agwozdowski can you add a pivotal task to track that?

@agwozdowski
Copy link
Contributor Author

@kowal added a story but it's a minor in my opinion. For now it's working in the same way like active-admin DESTROY action

@tsubik tsubik merged commit ef4a0eb into develop Sep 19, 2019
@tsubik tsubik deleted the feature/litigation-soft-delete branch September 23, 2019 12:31
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.

None yet

3 participants