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

Delete archived devices which have a null archived_at date. #258

Closed
wants to merge 1 commit into from

Conversation

timcowlishaw
Copy link
Contributor

Fixes #252.
For some reason, we've got some legacy devices which never had their archived_at date set, which are causing the deletion job to crash, meaning that neither they (nor archived devices with subsequent IDs) are deleted. This adds a test for the failure, and fixes so that all archived devices with a null archived_at are deleted. This shouldn't cause a problem in future as only old devices have the null archived_at date.

For some reason, we've got some legacy devices which never had their archived_at date set, which are causing the deletion job to crash, meaning that neither they (nor archived devices with subsequent IDs) are deleted. This adds a test for the failure, and fixes so that all archived devices with a null archived_at are deleted. This shouldn't cause a problem in future as only old devices have the null archived_at date.
@oscgonfer
Copy link
Contributor

Hi @timcowlishaw

Thinking of potential future implications of allowing null archived_at dates, I am afraid of what happens to the 24h grace period for a user that "changes its mind".

Would it be maybe be better to add a date to those that don't have it and have the normal workflow go through these devices?

In other words, if those null archived_at dates had a date, would it work normally? And, could we then consider these null bits a fluke?

@oscgonfer
Copy link
Contributor

For now we don't merge this PR, as we have added a timestamp manually. We will come back to this in the following days.

@oscgonfer
Copy link
Contributor

Hi @timcowlishaw

I think this is solved now:

smartcitizen_production=# select id, created_at, workflow_state, archived_at from devices where workflow_state = 'archived';
 id | created_at | workflow_state | archived_at 
----+------------+----------------+-------------
(0 rows)

smartcitizen_production=# select count(id), workflow_state from devices group by workflow_state;
 count | workflow_state 
-------+----------------
  7867 | active
(1 row)

smartcitizen_production=# select id, created_at, workflow_state, archived_at from devices where workflow_state = 'archived';
 id | created_at | workflow_state | archived_at 
----+------------+----------------+-------------
(0 rows)

OK for you to close the PR?

@timcowlishaw
Copy link
Contributor Author

timcowlishaw commented Oct 6, 2023 via email

@oscgonfer oscgonfer closed this Oct 6, 2023
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.

NoMethodError: undefined method `<' for nil:NilClass
2 participants