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

Code Quality: Remove hotlinking feature flag code #12021

Merged
merged 25 commits into from
Aug 16, 2022

Conversation

timarney
Copy link
Contributor

@timarney timarney commented Jul 26, 2022

Context

This feature has been enabled by default in v1.23.0.

Summary

Removes feature flags for:

videoPosterHotlinking
linkIconHotlinking
posterHotlinking

Relevant Technical Choices

To-do

User-facing changes

Testing Instructions

  • This is a non-user-facing change and requires no QA

This PR can be tested by following these steps:

  1. Verify that hotlinking for story poster, video poster, and link icon is available and enabled and works for both contributors and admins
  2. Other functionality should be left unchanged.

Reviews

Does this PR have a security-related impact?

No

Does this PR change what data or activity we track or use?

No

Does this PR have a legal-related impact?

No

Checklist

  • This PR addresses an existing issue and I have linked this PR to it in ZenHub
  • I have tested this code to the best of my abilities
  • I have verified accessibility to the best of my abilities (docs)
  • I have verified i18n and l10n (translation, right-to-left layout) to the best of my abilities
  • This code is covered by automated tests (unit, integration, and/or e2e) to verify it works as intended (docs)
  • I have added documentation where necessary
  • I have added a matching Type: XYZ label to the PR

Fixes #11997

@timarney timarney changed the title Code Quality: Remove mediaRecording feature flag code Code Quality: Remove hotlinking feature flag code Jul 26, 2022
@timarney timarney self-assigned this Jul 26, 2022
@timarney timarney added Group: Media Pod: WP Type: Code Quality Things that need a refactor, rewrite or just some good old developer ❤️ labels Jul 26, 2022
@timarney timarney marked this pull request as ready for review July 26, 2022 22:32
Comment on lines 298 to 295
{(hasUploadMediaAction || enablePosterHotlinking) && (
{hasUploadMediaAction && (
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here, with poster hotlinking always allowed we can remove this entire condition here, no?

Copy link
Contributor

@miina miina Aug 4, 2022

Choose a reason for hiding this comment

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

Not sure if you discussed this somewhere else that the comment was marked resolved, but since now enablePosterHotlinking is always true, wouldn't we always display this and can remove hasUploadMediaAction check as well?

@googleforcreators-bot
Copy link
Collaborator

googleforcreators-bot commented Jul 27, 2022

Plugin builds for 1c8b40d are ready 🛎️!

@swissspidy
Copy link
Collaborator

Looks like some tests are failing now. Haven't checked closely, perhaps they need updating.

@timarney
Copy link
Contributor Author

Looks like some tests are failing now. Haven't checked closely, perhaps they need updating.

expected document not to contain element, found <button data-testid="media-upload-button">Media Upload Button!</button> instead

      172 |     const uploadMediaButton = screen.queryByText('Media Upload Button!');
      173 |
    > 174 |     expect(uploadMediaButton).not.toBeInTheDocument();
          |                                   ^
      175 |   });

Looking into it.

@timarney
Copy link
Contributor Author

timarney commented Jul 27, 2022

Tested with a user who doesn't have upload permissions ---

Screen Shot 2022-07-27 at 6 23 19 AM

=====

I believe this is what we're expecting --- no upload button but can still hotlink.

@timarney
Copy link
Contributor Author

timarney commented Jul 27, 2022

Erroring on the MediaInput / ForwardRef

Element type is invalid: expected a string (for built-in components) or a class/function (for composite components) but got: undefined. You likely forgot to export your component from the file it's defined in, or you might have mixed up default and named imports.

Check the render method of `ForwardRef(MediaInput)`.

@GoogleForCreators GoogleForCreators deleted a comment from lgtm-com bot Jul 27, 2022
@timarney timarney removed the request for review from spacedmonkey July 28, 2022 08:53
@timarney timarney requested a review from miina August 4, 2022 09:10
includes/Experiments.php Show resolved Hide resolved
Comment on lines 298 to 295
{(hasUploadMediaAction || enablePosterHotlinking) && (
{hasUploadMediaAction && (
Copy link
Contributor

@miina miina Aug 4, 2022

Choose a reason for hiding this comment

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

Not sure if you discussed this somewhere else that the comment was marked resolved, but since now enablePosterHotlinking is always true, wouldn't we always display this and can remove hasUploadMediaAction check as well?

@timarney
Copy link
Contributor Author

timarney commented Aug 4, 2022

#12021 (comment)

Not sure if you discussed this somewhere else that the comment was marked resolved, but since now enablePosterHotlinking is always true, wouldn't we always display this and can remove hasUploadMediaAction check as well?

It was flagged for removal but removing seems to be causing other issues -- test failures + this error #12021 (comment) thinking there's something deeper into one of the child components around the can user upload perms.

@miina
Copy link
Contributor

miina commented Aug 4, 2022

#12021 (comment)

Not sure if you discussed this somewhere else that the comment was marked resolved, but since now enablePosterHotlinking is always true, wouldn't we always display this and can remove hasUploadMediaAction check as well?

It was flagged for removal but removing seems to be causing other issues -- test failures + this error #12021 (comment) thinking there's something deeper into one of the child components around the can user upload perms.

Hm, okay. I suppose we can leave as is then. Just by reading the code I assumed that the whole component could always displayed but canUpload should only be true in case of hasUploadMediaAction.

@timarney
Copy link
Contributor Author

timarney commented Aug 4, 2022

Hm, okay. I suppose we can leave as is then. Just by reading the code I assumed that the whole component could always displayed but canUpload should only be true in case of hasUploadMediaAction.

I'll take another look --- see if I can spot anything

Otherwise contributors without upload permissions cannot update the story poster via hotlinking.

Updates tests.
@swissspidy
Copy link
Collaborator

@timarney That hasUploadMediaAction must go away. Otherwise contributors cannot update the story poster in the pre-publish dialog. Notice the missing button here:

Screenshot 2022-08-09 at 11 10 10

Quickly addressed it in 96029c0 but there might be more tests that need updating.

@timarney
Copy link
Contributor Author

timarney commented Aug 9, 2022

@swissspidy any idea why removing that causes issues with the MediaInput (at least in the tests).

detail: Error: Element type is invalid: expected a string (for built-in components) or a class/function (for composite components) but got: undefined. You likely forgot to export your component from the file it's defined in, or you might have mixed up default and named imports.
 
 Check the render method of `ForwardRef(MediaInput)`.

@swissspidy
Copy link
Collaborator

That test renders the pre-publish modal without defining MediaUpload in the configValue in its arrange() function. See how the storyPreview.js test does it:

MediaUpload: ({ onSelect }) => (
<button
data-testid="media-upload-button"
onClick={() => onSelect(newPoster)}
>
{'Media Upload Button!'}
</button>
),
}}

Now that we removed this condition, the MediaUpload component is always rendered by this test. But because it's undefined there's an error.

@swissspidy swissspidy requested a review from miina August 9, 2022 13:46
@swissspidy swissspidy merged commit b2b52cd into main Aug 16, 2022
@swissspidy swissspidy deleted the fix/11997-remove-hotlinking-feature-flag branch August 16, 2022 12:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Group: Media Type: Code Quality Things that need a refactor, rewrite or just some good old developer ❤️
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Code Quality: Remove hotlinking feature flag code
4 participants