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
Added missing archivedAt to featureSchema #1779
Conversation
Added archived_at to db
# Conflicts: # src/test/e2e/api/openapi/__snapshots__/openapi.e2e.test.ts.snap
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
Coverage report
Show new covered files 🐣
Show files with reduced coverage 🔻
Test suite run success992 tests passing in 166 suites. Report generated by 🧪jest coverage report action from eb24007 |
exports.up = function (db, callback) { | ||
db.runSql( | ||
` | ||
ALTER TABLE features ADD archived_at date; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this use TIMESTAMP WITH TIME ZONE
instead of date
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that would align better with rest of the schema.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
const row = await this.db(TABLE) | ||
.where({ name }) | ||
.update({ archived: true }) | ||
.update({ archived_at: now }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we keep setting archived
as well, while we have both columns?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont think so, because we dont want to be dependant on manually setting the column. We want the archived
value to be calculated always from archived_dt
, so if we eventually removed archived
column, then we know that everything has been working fine based on just the date column.
src/lib/db/feature-toggle-store.ts
Outdated
if (archived) { | ||
queryBuilder.whereNotNull('archived_at'); | ||
} else { | ||
queryBuilder.whereNull('archived_at'); | ||
} | ||
return queryBuilder; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could perhaps line up the conditions:
return archived
? queryBuilder.whereNotNull('archived_at')
: queryBuilder.whereNull('archived_at');
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, we can go with shortif.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -2,11 +2,10 @@ | |||
id: events | |||
title: /api/admin/events | |||
--- | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would revert this file, unless there are any non-formatting changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
src/lib/types/model.ts
Outdated
@@ -53,6 +54,7 @@ export interface FeatureToggleDTO { | |||
export interface FeatureToggle extends FeatureToggleDTO { | |||
project: string; | |||
lastSeenAt?: Date; | |||
variants?: IVariant[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like variants
should already be here, from FeatureToggleDTO
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
.expect(200) | ||
.expect((res) => { | ||
expect(res.body.features.length).toEqual(3); | ||
expect(res.body.features[0].archivedAt).toBeTruthy(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could check all the features:
.expect((res) => {
expect(res.body.features.length).toEqual(3);
expect(res.body.features.every((f) => f.archived)).toEqual(true);
expect(res.body.features.every((f) => f.archivedAt)).toEqual(true);
});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I was designing thinking in a mind that a test should usually test single functionality, but either is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed it to single test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
archivedAt
column to featuresFeatureToggleStore
, which are covered byFeatureToggleStore.getAll
methodarchived
column will not be used anymore, but as discussed in daily standup, not yet deleted. Will be deleted in future, if everything goes smoothly.