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

Add lastAccessed field to map table #296

Conversation

sorenjohanson
Copy link
Collaborator

closes #286

@JannikStreek
Copy link
Member

Please don't forget to generate the migration file for this, see https://github.com/b310-digital/teammapper/blob/main/teammapper-backend/README.md.

@JannikStreek
Copy link
Member

please also add the use of this field to the delete handler in the service (deleteOutdatedMaps), so that we use the lastAccessed field on map as a default and as a backup the lastModified field (existing logic). We can also do this in a new PR?

@sorenjohanson
Copy link
Collaborator Author

please also add the use of this field to the delete handler in the service (deleteOutdatedMaps), so that we use the lastAccessed field on map as a default and as a backup the lastModified field (existing logic). We can also do this in a new PR?

I'm working on that right now, it's just that I need to restructure the queries a bit to work with the new field, so it'll take some time.

@sorenjohanson sorenjohanson marked this pull request as ready for review June 21, 2024 09:28
@JannikStreek
Copy link
Member

For documentation purposes:

  • When a map is created, createdAt will be filled, accessedAt will be nil.
  • When accessed for the first time (after creation, first reload or load by user user of map), accessedAt will be set.
  • During the migration, old maps will have empty accessedAt and createdAt fields. lastModified will be still available and this field is essentially a createdAt field as the date is not updated after creation.

@JannikStreek
Copy link
Member

JannikStreek commented Sep 12, 2024

ok there are actually >300 maps on our server, that do not have any nodes. Interestingly, all these maps are not old but were created in September - so there seems to be a bug not creating the root node or deleting the root node.

But this is a bug for a different issue. However, we should still include this case, that a map does not contain any nodes. => nodeLastUpdatedAt = null AND GREATER(map.lastModified, map.lastAccessed) ...

The following test cases are the most relevant:

  • Only lastModified is set on a map (either too old or recent enough) with one node that was either modified before and after the lastModified date.
  • lastModified AND lastAccess are set (one being older than the other) + with one node existing, being modified at a later point and at a newer point in time
  • A map that has no nodes at all for some reason => only look at lastModified as a reference. I also thought about deleting those directly, but for debugging purposes it makes sense to keep them for some time.

@@ -152,6 +208,33 @@ describe('MapsController', () => {
expect(await nodesRepo.findOne({ where: { id: node.id } })).not.toBeNull()
})

it('deletes a map which has outdated nodes and outdated lastAccess', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

also add a test (if not given, can't find it right now): Outdated modifedAt, recent nodes => do not delete. This is how its working right now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is covered by it('does not delete a map that contains a recent node'), where lastAccessed is set to 2019 but the node is set to today using new Date().

@@ -233,19 +239,9 @@ export class MapsService {
'lastmodifiednode.nodeMapid = map.id'
)
.where(
// delete all maps that have nodes that were last updated after afterDays
"(lastmodifiednode.lastUpdatedAt + (INTERVAL '1 day' * :afterDays)) < :today",
"(GREATEST(map.lastAccessed, map.lastModified, lastmodifiednode.lastUpdatedAt) + (INTERVAL '1 day' * :afterDays)) < :today",
Copy link
Member

Choose a reason for hiding this comment

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

thats a cool discovery that this takes more than 2 arguments, makes it way easier 👍

@sorenjohanson
Copy link
Collaborator Author

sorenjohanson commented Sep 16, 2024

Dates are now set explicitly in every test, in addition to the following format changes:

An old date is always set to 2021-01-01.
A recent date is always set to 2024-09-01.

If we're testing recent dates, system time will always be set exactly to the same date.
On the other hand, if we're testing old dates, system time will always be set to old date + 30 days, which will always result in system time being set to 2021-01-31.

@JannikStreek
Copy link
Member

looks good, I also tested everything locally with cross running every minute and 1 day as delete threshold. Could not find any problems.

@JannikStreek JannikStreek merged commit 8fbb0fa into b310-digital:main Sep 16, 2024
5 checks passed
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.

Add lastAccessed field to map table
2 participants