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(Documents): Expire non required documents #358

Merged
merged 1 commit into from
Nov 10, 2022
Merged

feature(Documents): Expire non required documents #358

merged 1 commit into from
Nov 10, 2022

Conversation

santostiago
Copy link
Contributor

Task.

This PR adds "not-required" documents to the list of documents to expire.

Pull request checklist

Please check if your PR fulfills the following requirements:

  • Make sure you are requesting to pull a topic/feature/bugfix branch (right side). Don't request your master!
  • Make sure you are making a pull request against the develop branch (left side). Also you should start your branch off our develop.
  • Check the commit's or even all commits' message styles matches our requested structure.
  • Check your code additions will fail neither code linting checks nor unit test.

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

Only valid documents get expired

Issue Number: 183467740

What is the new behavior?

Not required documents also get expired.

Does this introduce a breaking change?

  • Yes
  • No

Other information

@santostiago santostiago self-assigned this Nov 6, 2022
tsubik
tsubik reviewed Nov 7, 2022
View reviewed changes
app/models/operator_document.rb Outdated
}

enum status: { doc_not_provided: 0, doc_pending: 1, doc_invalid: 2, doc_valid: 3, doc_expired: 4, doc_not_required: 5 }
enum uploaded_by: { operator: 1, monitor: 2, admin: 3, other: 4 }
enum source: { company: 1, forest_atlas: 2, other_source: 3 }

NON_HISTORICAL_ATTRIBUTES = %w[id attachment updated_at created_at].freeze
EXPIRABLE_STATUSES = statuses.slice(:doc_valid, :doc_expired).values.freeze
Copy link
Collaborator

Choose a reason for hiding this comment

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

%w[doc_valid doc_expired] or %i[doc_valid doc_expired] would be enough and it should work I think. Rails itself should change those values to integer.

tsubik
tsubik reviewed Nov 7, 2022
View reviewed changes
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.

You can add another spec, pretty simple for #expire_documents in operator_document_spec.rb just to add another document with different status

tsubik
tsubik approved these changes Nov 7, 2022
View reviewed changes
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.

Other than missing spec, I think all good. Expiration task should that we already have running daily should expire those not required documents.

@santostiago
feature(Documents): Expire non required documents
759bcb7 
This commit adds "not-required" documents to the list of documents to expire.
@santostiago santostiago force-pushed the feat-expire_non_required_documents branch from 582b799 to 759bcb7 Compare November 8, 2022 21:25
@santostiago
Copy link
Contributor Author

@tsubik I updated the PR to include tests.
I also realized we were not expiring gov documents, so I added them to the job (I'll confirm it with Sophie, but I think we weren't expiring them by mistake, and not by design).

@santostiago
Copy link
Contributor Author

Also, we should (later) refactor the gov documents and operator documents to include a documents module. Like 50% of the code is the same.

tsubik
tsubik approved these changes Nov 9, 2022
View reviewed changes
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 great. New specs are very well organized. I'm always struggling with that ;)

app/admin/required_gov_document.rb
@@ -20,12 +20,11 @@
rd.deleted_at.nil?
end
column 'required_gov_document_group' do |rd|
rd.required_operator_document_group&.name
rd.required_gov_document_group&.name
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

spec/factories/gov_documents.rb
current { true }

transient do
force_status { nil }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wouldn't work if we just set status? Anyway, interesting use of transient.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I always do it like this. I'm not sure if status would work.

@santostiago santostiago merged commit ea08a4c into develop Nov 10, 2022
@santostiago santostiago deleted the feat-expire_non_required_documents branch November 10, 2022 19:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tsubik tsubik tsubik approved these changes

Assignees

@santostiago santostiago

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@santostiago @tsubik