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

🐛 Don't show external label in label selector #161

Merged
merged 3 commits into from
Aug 29, 2024

Conversation

foysalit
Copy link
Contributor

When taking label action, the label selector shows all labels that are currently applied on the subject and as a result, any label applied by an external labeler can be unselected. However, in reality, this doesn't do anything since one can not remove a label applied by an external labeler.

To reduce confusion around this, this PR removes external labelers from label selector.

Copy link

render bot commented Aug 13, 2024

@foysalit foysalit requested a review from devinivy August 13, 2024 21:56
@foysalit foysalit force-pushed the disallow-editing-external-label branch from 9ae2e40 to 490ebb4 Compare August 18, 2024 11:39
@@ -37,7 +37,7 @@
"labels":[
{
"ver":1,
"src":"did:plc:4wke3qtisoohr7tf6vfhe5r2",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

these had to be changed because we never used the src value for any functionality being tested, now that we're hitting that path in tests, we need to make sure the src matches with the test dataset.

(label) => !isSelfLabel(label),
)}
defaultLabels={currentLabels.filter((label) => {
const serviceDid = client.getServiceDid()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it possible that this "DID" contains the service id #atproto_labeler, yet l.src below will omit that? Or does getServiceDid() return just the DID without the service id on the end?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah right yeah that needs to be trimmed out thanks!

Comment on lines 674 to 676
const isExternalLabel = allLabels.find((l) => {
return l.val === label && l.src !== serviceDid
})
Copy link
Collaborator

Choose a reason for hiding this comment

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

No biggie, but if we're trying to get a bool out (as indicated by the name isExternalLabel) then some() does that for us.

Suggested change
const isExternalLabel = allLabels.find((l) => {
return l.val === label && l.src !== serviceDid
})
const isExternalLabel = allLabels.some((l) => {
return l.val === label && l.src !== serviceDid
})

@arcalinea arcalinea temporarily deployed to disallow-editing-external-label - ozone-staging PR #161 August 29, 2024 17:52 — with Render Destroyed
@foysalit
Copy link
Contributor Author

made the changes requested and merging but happy to make other changes if needed.

@foysalit foysalit merged commit 3ba90cc into main Aug 29, 2024
3 checks passed
@matthieusieben matthieusieben deleted the disallow-editing-external-label branch November 15, 2024 14:55
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.

3 participants