Remember whether the welcome screen was seen #2375
Conversation
group-income
|
||||||||||||||||||||||||
| Project |
group-income
|
| Run status |
|
| Run duration | 09m 29s |
| Commit |
|
| Committer | Ricardo Iván Vieitez Parra |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
10
|
Skipped
|
0
|
Passing
|
111
|
| @@ -52,6 +54,14 @@ export default ({ | |||
| toDashboard () { | |||
| if (this.isButtonClicked) return | |||
| this.isButtonClicked = true | |||
| const groupContractID = this.currentGroupId | |||
There was a problem hiding this comment.
I'm wondering whether this should be here, or if instead it should be done on the /dashboard page on onMounted. Maybe it doesn't matter.
There was a problem hiding this comment.
Pros of doing it here: it's only done once (for users). Pros of doing it in the /dashboard page: we don't need to change the commands.js in the tests. Cons of doing it in the /dashboard page: need to do this check every time.
corrideat
force-pushed
the
welcome-page-vuex
branch
from
2ce1c8c
to
83a69ce
Compare
| @@ -283,8 +283,7 @@ sbp('chelonia/defineContract', { | |||
| throw new Error(`Cannot leave group ${groupContractID} because the reference hash does not match the latest`) | |||
| } | |||
|
|
|||
| state.groups[groupContractID].hasLeft = true | |||
| delete state.groups[groupContractID].inviteSecret | |||
| state.groups[groupContractID] = { hash: reference, hasLeft: true } | |||
There was a problem hiding this comment.
There might be other attributes on groups that you're overwriting here that shouldn't be overwritten.
There was a problem hiding this comment.
There aren't. This is when leaving a group, so we should keep the attributes there at a minimum.
There was a problem hiding this comment.
It's true that there aren't at the moment, but in the future if more attributes are added then developers must remember to update this code to make sure nothing breaks. Whereas the way it worked before didn't need as much attention.
There was a problem hiding this comment.
but in the future if more attributes are added then developers must remember to update this code to make sure nothing breaks
Nothing should break because these are chatroom that we're not part of. Note that attributes are already overwritten when joining a chatroom. The only reason we keep these values around at all (before we deleted the whole thing) is for accounting purposes, so that re-joining works correctly.
then developers must remember to update
I think this is a good thing. You shouldn't need attributes from chatroom you're not a part of. The only exception I can think of is for a future feature that allows you to 'block' a chatroom (to prevent re-joining), in which case it makes sense to be explicit.
There was a problem hiding this comment.
Either way, making things as they were before still means that developers need to pay attention to this. If a foo attribute were to be added, which is not used after having left, it needs to have a corresponding delete here.
There was a problem hiding this comment.
ok that last comment makes sense... can you add a comment here explaining that?
| ]), | ||
| ...mapState([ | ||
| 'currentGroupId' | ||
| 'currentGroupId', | ||
| '' |
| const state = sbp('state/vuex/state') | ||
| return sbp('gi.actions/identity/setGroupAttributes', { | ||
| contractID: state.loggedIn.identityContractID, | ||
| data: { | ||
| groupContractID: state.currentGroupId, | ||
| attributes: { seenWelcomeScreen: true } | ||
| } | ||
| }).then(() => router.push({ path: '/dashboard' })) |
There was a problem hiding this comment.
Is there a way this can be DRY'd? (Repeated above)
There was a problem hiding this comment.
I'm not entirely sure. It could maybe be a Cypress command.
No description provided.