fix: Rework initial settings storage on invitee - #1123
Conversation
There was a problem hiding this comment.
Do we have a test that actually checks that this data is being used? Maybe check the code coverage to see whether the lines where we attempted to read the backupInfo are actually being called by test code?
Note that we will still need the re-index to happen along with the deletion of the projectSettings index table - the "bad" data will still be in there for existing projects, this just stops it for newly invited devices going forwards.
A few things to address in the comments.
Also, obviously, tests need to pass.
| ) | ||
| }) | ||
|
|
||
| test('Set project settings after being invited', async (t) => { |
There was a problem hiding this comment.
Is this test failing prior to the fix? Could you maybe separate this into a separate commit so we can see CI confirmation of the failing test?
There was a problem hiding this comment.
Yes it was failing and would like have failed since version 4.1.0. I'll do a rebase
There was a problem hiding this comment.
Thanks. In a perfect world, for bug fixes, it's always good to start with a commit with a failing test, to confirm on CI that the test actually works, followed by the fix. I've made mistakes before writing tests that don't actually fail in all circumstances.
There was a problem hiding this comment.
Yeah I'll make sure to make a commit to track that fact next time. In the past I've preferred making my first commit as close to "working" as possible but I see the value in starting with a failing test commit so I can adjust.
There was a problem hiding this comment.
Rebased to make the test commit separate from the fix
|
Regarding whether we have tests that actually check for getProjectStats, that's happening inside the list projects and leave project tests in addition to the $getProjectSettings test |
|
Instead of dropping the entire project settings table I have the projects remove their entry from it on re-index |
cba6a17 to
f2aeea2
Compare
|
Closed in favor of #1125 |
2 participants
Does away with the previous approach of using old unix timestamps with the hope of them being overwritten to instead store project data from the invite in a separate table and to fall back to it if we don't have a synced value
Closes #1122