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.

Sign up for GitHub

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

Jump to bottom

(feature): legislations update - soft delete + batch actions (archive/destroy) #55

Merged
merged 20 commits into from
Sep 12, 2019
Merged

(feature): legislations update - soft delete + batch actions (archive/destroy) #55

merged 20 commits into from
Sep 12, 2019

Conversation

kowal
Copy link
Contributor

@kowal kowal commented Sep 10, 2019 •
edited
Loading

Summary

For Legislations resource:

  • added soft-delete using discard (when deleted, records are not visible anywhere in the UI)
  • added Archive & Delete batch actions
  • on the litigation show page, display archived legislations with "ARCHIVE" prefix

Deleting Legislation means:

  • discarding it (it stays in DB)
  • discarding associated documents and events
  • clearing associated litigations and targets

Archiving Legislations means:

  • simply updating its visibility_status to archived so it can be unarchived to another state at any time

Zrzut ekranu 2019-09-10 o 22 56 16

kowal
kowal commented Sep 10, 2019
View reviewed changes
app/admin/legislations.rb
{alert: 'Could not archive selected Legislations'}
end

redirect_to collection_path(scope: 'archived'), results
Copy link
Contributor Author

Choose a reason for hiding this comment

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

set scope to archive after batch-archiving records

kowal
kowal commented Sep 10, 2019
View reviewed changes
app/commands/command/batch/archive.rb
end

def call
@collection
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we want to perform some extra checks before (batch)-archiving? Are there any cases when records (legislations in particular) should not have option to be archived? Those could be modeled using AM validations, similar as in CsvDataUpload

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm thinking that this will be safeguarded by the authorization rules, where not everyone will have access to all features. (I know, I need to finish that bit!). So for now, I wouldn't spend too much time thinking. As it will be brought in or handled from a different side.

kowal
kowal commented Sep 10, 2019
View reviewed changes
app/models/legislation.rb Outdated Show resolved Hide resolved
@kowal kowal changed the title [IWP] Feature/archive soft delete [WIP] Feature/archive soft delete Sep 10, 2019
@kowal kowal requested a review from simaob September 10, 2019 21:05
@kowal kowal changed the title [WIP] Feature/archive soft delete (feature): legislations update - soft delete + batch actions (archive/destroy) Sep 10, 2019
@kowal kowal marked this pull request as ready for review September 10, 2019 21:07
@simaob
Copy link
Contributor

simaob commented Sep 11, 2019

Good job so far!

Comment: the deleted success message should probably say "deleted" instead of "destroyed", just for language sake.

Answer to:

deleting associated documents and events (⚠️ @simaob do we really want this?)

I'm wondering if we should discard/soft delete events, but delete documents (as they would take space in the Google Cloud Storage). What do you think? We can also confirm with them and potentially just keep the documents, if it's not too expensive.

But overall this is the way!

@simaob
Copy link
Contributor

simaob commented Sep 11, 2019

Having discussed this over slack we decided to go with discarding documents and events.

@kowal
Copy link
Contributor Author

kowal commented Sep 11, 2019

Comment: the deleted success message should probably say "deleted" instead of "destroyed", just for language sake.

it is already :)

@kowal
Copy link
Contributor Author

kowal commented Sep 11, 2019

I also need to overwrite batch-delete action - default one still destroys objects 😅

simaob
simaob reviewed Sep 12, 2019
View reviewed changes
Copy link
Contributor

@simaob simaob left a comment

Choose a reason for hiding this comment

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

Great job! =) This is nearly there. Just one open question, and afterwards it can be merged.

app/commands/command/destroy/legislation.rb Show resolved Hide resolved
@simaob simaob self-requested a review September 12, 2019 15:27
simaob
simaob approved these changes Sep 12, 2019
View reviewed changes
Copy link
Contributor

@simaob simaob left a comment

Choose a reason for hiding this comment

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

merging!

@simaob simaob merged commit cea43c4 into develop Sep 12, 2019
@simaob simaob deleted the feature/archive-soft-delete branch September 12, 2019 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simaob simaob simaob approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kowal @simaob