refactor: remove share status in favour of syncEnabled #10421
Conversation
|
Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes. |
JammingBen
force-pushed
the
remove-share-status
branch
3 times, most recently
from
36ef6ec to
d9a7d0b
Compare
JammingBen
force-pushed
the
remove-share-status
branch
4 times, most recently
from
381d60f to
51cf938
Compare
JammingBen
force-pushed
the
remove-share-status
branch
from
51cf938 to
f5150ec
Compare
| resource.status = parseInt(share.state) | ||
| resource.hidden = share.hidden === 'true' || share.hidden === true | ||
| ;(resource as IncomingShareResource).syncEnabled = parseInt(share.state) === 0 | ||
| ;(resource as IncomingShareResource).hidden = share.hidden === 'true' || share.hidden === true |
There was a problem hiding this comment.
As you already implemented type predicates, can't we somehow narrow the type down here properly reasonably instead of just casting it?
https://www.typescriptlang.org/docs/handbook/2/narrowing.html#using-type-predicates
There was a problem hiding this comment.
This will be removed later today, so I decided to not invest any time 😄
Edit: see #10426
| return false | ||
| } | ||
| if (isIncomingShareResource(resources[0]) && !resources[0].syncEnabled) { | ||
| return false |
There was a problem hiding this comment.
sweeeeeeeeeeeeeeeeeeeeeeeeeeeeet
| resource, | ||
| status: resource.status, | ||
| status: resource.syncEnabled ? 0 : 2, |
There was a problem hiding this comment.
This is probably for the time being until we're moving towards the graph client completely?
There was a problem hiding this comment.
This will be removed as one of the next steps, so again I decided to not invest much time into making it more readable.
| client, | ||
| hidden = undefined, | ||
| spaces = [], | ||
| fullShareOwnerPaths = false |
There was a problem hiding this comment.
Reminds me - have you tried this with fullShareOwnerPaths? 🙈 👀
There was a problem hiding this comment.
Not yet, it will probably break eventually with the transition to sharing NG (or tbh I think the first PR already broke it 😅 ). But I'd like to get this transition going first, especially because code paths like these will change in a few days anyway.
I added it to the epic though: #10418
Yeah, I was thinking about the same but decided for the clean cut here because the remaining magic numbers will be removed as one of the next steps anyway (which is basically the |
JammingBen
force-pushed
the
remove-share-status
branch
from
3443f1c to
8fcd572
Compare
|
Description
Remove share status in favour of the
syncEnabledproperty since sharing NG won't have share statuses anymore. Also renames the actions and test steps accordingly.The remaining places where we still use the status (e.g.
triggerShareAction) are deprecated and will be removed with one of the next changes.In addition to that I decided to introduce
OutgoingShareResourceandIncomingShareResourceinterfaces that extendShareResoruce, since some properties are only available on incoming share resources. This makes type checking easier and even allows us to kill some of the pesky route checks (e.g.IncomingShareResource== "Shared with me" page).Related Issue
Types of changes