web-stories-wp icon indicating copy to clipboard operation
web-stories-wp copied to clipboard

Code Quality: Remove hotlinking feature flag code

Open timarney opened this issue 3 years ago • 11 comments

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

  • [x] This PR addresses an existing issue and I have linked this PR to it in ZenHub
  • [x] 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
  • [x] I have added a matching Type: XYZ label to the PR

Fixes #11997

timarney avatar Jul 26 '22 11:07 timarney

Plugin builds for 1c8b40dd0124ff08933ee60f27be9189a03ce403 are ready :bellhop_bell:!

googleforcreators-bot avatar Jul 27 '22 09:07 googleforcreators-bot

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

swissspidy avatar Jul 27 '22 09:07 swissspidy

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 avatar Jul 27 '22 09:07 timarney

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 avatar Jul 27 '22 10:07 timarney

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)`.

timarney avatar Jul 27 '22 10:07 timarney

https://github.com/GoogleForCreators/web-stories-wp/pull/12021#discussion_r937569971

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 https://github.com/GoogleForCreators/web-stories-wp/pull/12021#issuecomment-1196559746 thinking there's something deeper into one of the child components around the can user upload perms.

timarney avatar Aug 04 '22 09:08 timarney

#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.

miina avatar Aug 04 '22 10:08 miina

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

timarney avatar Aug 04 '22 11:08 timarney

@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 96029c07d382d2a2214b9d3cfa0f298eb5a3af14 but there might be more tests that need updating.

swissspidy avatar Aug 09 '22 09:08 swissspidy

@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)`.

timarney avatar Aug 09 '22 09:08 timarney

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:

https://github.com/GoogleForCreators/web-stories-wp/blob/a6b3cb6047426b680db026413ca69588b13e99d0/packages/story-editor/src/components/publishModal/test/storyPreview.js#L60-L68

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 avatar Aug 09 '22 09:08 swissspidy