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

Unlink as coverage #1847

Merged
merged 22 commits into from
Sep 13, 2024
Merged

Unlink as coverage #1847

merged 22 commits into from
Sep 13, 2024

Conversation

thecalcc
Copy link
Contributor

@thecalcc thecalcc commented Aug 31, 2023

SDESK-7017

@thecalcc thecalcc requested a review from tomaskikutis August 31, 2023 10:14
@tomaskikutis
Copy link
Member

can you fix CI before assigning for review? :) image

@petrjasek petrjasek added this to the 2.8 milestone Sep 11, 2023
Copy link
Member

@tomaskikutis tomaskikutis left a comment

Choose a reason for hiding this comment

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

CI is still failing :)

client/planning-extension/src/extension.ts Outdated Show resolved Hide resolved
Base automatically changed from authoring-react-post-broadcasting to develop February 6, 2024 13:29
@thecalcc
Copy link
Contributor Author

Update package-json to use cc develop

@tomaskikutis
Copy link
Member

which ticket is this handling?

@thecalcc
Copy link
Contributor Author

which ticket is this handling?

SDESK-7017

onTrigger: () => {
const superdeskArticle = superdesk.entities.article;

if (
Copy link
Member

Choose a reason for hiding this comment

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

Did you take these conditions from somewhere else? In order not to duplicate it, we should add canUnlinkAscoverage function and expose it via extensions bridge.

Copy link
Contributor Author

@thecalcc thecalcc Sep 13, 2024

Choose a reason for hiding this comment

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

The thing is, we can't add it 1-to-1, because in the angular world permissions work under the hood, so if we do add it it'll still be chopped up. Personally I don't think it's necessary when this is the case, so we can keep it like this

Copy link
Member

Choose a reason for hiding this comment

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

add the comment about keeping it up to date

onTrigger: () => {
const superdeskArticle = superdesk.entities.article;

if (
Copy link
Member

Choose a reason for hiding this comment

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

add the comment about keeping it up to date

@thecalcc thecalcc enabled auto-merge (squash) September 13, 2024 09:40
@tomaskikutis
Copy link
Member

one more thing, add TAG: AUTHORING-ANGULAR where it's registered on the angular side. I've started adding the tags too so when time comes to remove angular based authoring, we can search for tags and easily find all related code that needs to be removed as well.

@thecalcc thecalcc merged commit a480add into develop Sep 13, 2024
26 of 27 checks passed
@thecalcc thecalcc deleted the unlink-as-coverage branch September 13, 2024 10:11
@tomaskikutis
Copy link
Member

did you see my last comment?

@@ -103,6 +103,8 @@ function configurePlanning(superdesk) {
!['killed', 'recalled', 'unpublished', 'spiked', 'correction'].includes(item.state);
}],
})

// TAG: AUTHORING-ANGULAR
Copy link
Member

Choose a reason for hiding this comment

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

image

I'm wondering about that external-app action.

image

It looks like the same action also appears in the topbar. Could you double check whether we also have it in authoring-react?

Copy link
Member

Choose a reason for hiding this comment

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

@thecalcc have you looked into this yet?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm doing so yep, but I plan to address it in a different PR. haven't forgotten about it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm doing so yep, but I plan to address it in a different PR. haven't forgotten about it

In short, looks like this is some filtering based on which we calculate whether to show the action in the three dots menu or not

@thecalcc thecalcc mentioned this pull request Sep 17, 2024
@petrjasek petrjasek removed this from the 2.8 milestone Sep 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants