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.

Sign up for GitHub

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

Social: Fix media validation deadlock #38933

Merged
merged 9 commits into from
Aug 28, 2024
Merged

Social: Fix media validation deadlock #38933

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
0628513
Show SharePostForm if there is attached media
gmjuhasz Aug 16, 2024
a9724e1
changelog
gmjuhasz Aug 16, 2024
ae7c3d0
Merge branch 'trunk' into fix/social-media-validation-deadlock
gmjuhasz Aug 21, 2024
0e3167c
Show form if there is any validation error
gmjuhasz Aug 21, 2024
a806222
Use only !isConvertible
gmjuhasz Aug 21, 2024
9f3c520
Use validationErrors
gmjuhasz Aug 22, 2024
3f27ace
Merge branch 'trunk' into fix/social-media-validation-deadlock
gmjuhasz Aug 26, 2024
0eb7571
Add a small comment to explain why we do that
gmjuhasz Aug 26, 2024
3e475dd
Merge branch 'trunk' into fix/social-media-validation-deadlock
gmjuhasz Aug 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions projects/js-packages/publicize-components/changelog/fix-social-media-validation-deadlock
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Significance: minor
Type: fixed

Fixed a deadlock with media validation and media picker
24 changes: 20 additions & 4 deletions projects/js-packages/publicize-components/src/components/form/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@ import { Disabled, PanelRow } from '@wordpress/components';
import { useSelect } from '@wordpress/data';
import { Fragment } from '@wordpress/element';
import { usePublicizeConfig } from '../../..';
import useAttachedMedia from '../../hooks/use-attached-media';
import useFeaturedImage from '../../hooks/use-featured-image';
import useMediaDetails from '../../hooks/use-media-details';
import useMediaRestrictions from '../../hooks/use-media-restrictions';
import useSocialMediaConnections from '../../hooks/use-social-media-connections';
import { store as socialStore } from '../../social-store';
import { ThemedConnectionsModal as ManageConnectionsModal } from '../manage-connections-modal';
Expand All @@ -25,8 +29,22 @@ import { SharePostForm } from './share-post-form';
* @return {object} - Publicize form component.
*/
export default function PublicizeForm() {
const { hasConnections, hasEnabledConnections } = useSocialMediaConnections();
const { hasConnections, hasEnabledConnections, connections } = useSocialMediaConnections();
const { isPublicizeEnabled, isPublicizeDisabledBySitePlan } = usePublicizeConfig();
const { attachedMedia } = useAttachedMedia();
const featuredImageId = useFeaturedImage();

const mediaId = attachedMedia[ 0 ]?.id || featuredImageId;
Copy link
Member

Choose a reason for hiding this comment

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

Why should featured image be a part of this logic?

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 thought that if the featured image is invalid, you still get the deadlock in the editor on the jetpack tab, until you remove the featured image. If we use it here, they can also use the attached media to upload a working image without having to change the featured image. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough. Thank you for the explanation.

const { validationErrors, isConvertible } = useMediaRestrictions(
connections,
useMediaDetails( mediaId )[ 0 ]
);

const showSharePostForm =
isPublicizeEnabled &&
( hasEnabledConnections ||
attachedMedia.length > 0 ||
( Object.keys( validationErrors ).length !== 0 && ! isConvertible ) );
Copy link
Member

@manzoorwanijk manzoorwanijk Aug 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

In what case will there be no validation errors when the media is not convertible?

I mean if the media is not convertible, won't there already be validation errors?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So you say we should only look for ! isConvertible in that part of the logic?

Copy link
Member

Choose a reason for hiding this comment

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

I am not sure but my concern is that if the media is not convertible, we should already show/have validation errors. If that is not the case then we may need to fix that first, otherwise, yes, we may only look for ! isConvertible in that part of the logic.

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 am not sure but my concern is that if the media is not convertible, we should already show/have validation errors.

Yeah we do, I think it's true that if something is not convertible, that means we have at least 1 error

Copy link
Member

Choose a reason for hiding this comment

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

Cool. Thank you for the confirmation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We only use that here too: MediaValidationNotices I'll update that


const { useAdminUiV1, featureFlags } = useSelect( select => {
const store = select( socialStore );
Expand Down Expand Up @@ -57,9 +75,7 @@ export default function PublicizeForm() {

{ ! isPublicizeDisabledBySitePlan && (
<Fragment>
{ isPublicizeEnabled && hasEnabledConnections && (
<SharePostForm analyticsData={ { location: 'editor' } } />
) }
{ showSharePostForm && <SharePostForm analyticsData={ { location: 'editor' } } /> }
</Fragment>
) }
</Wrapper>
Expand Down
Loading