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

Cleanup: Remove unused editor code #22906

Closed
twstokes opened this issue Mar 26, 2024 · 14 comments · Fixed by #23043
Closed

Cleanup: Remove unused editor code #22906

twstokes opened this issue Mar 26, 2024 · 14 comments · Fixed by #23043

Comments

@twstokes
Copy link
Contributor

twstokes commented Mar 26, 2024

Research and confirm if we can remove the following from the project:

  • EditPageViewController(homepage:)
  • PostEditorAction.continueFromHomepageEditing
  • ⚠️ AztecVerificationPromptHelper
@twstokes twstokes self-assigned this Mar 26, 2024
@dangermattic
Copy link
Collaborator

Thanks for reporting! 👍

@kean
Copy link
Contributor

kean commented Mar 26, 2024

Please, don't remove these just yet. I'll do it on Monday as long with some other unused code.

@twstokes
Copy link
Contributor Author

No problem @kean - my plan is just to help research and confirm.

@twstokes
Copy link
Contributor Author

EditPageViewController(homepage:) can go since it has no more callers.

@kean
Copy link
Contributor

kean commented Mar 29, 2024

Nice!

@twstokes
Copy link
Contributor Author

PostEditorAction.continueFromHomepageEditing can go because (from what I can tell) it depends on isHomePageEditor being true, and that's only set via EditPageViewController(homepage:). That then cascades to undoing this project which lands the user in the editor after creating a new site.

Personally I'm not sure why we chose to stop supporting this functionality as it appears quite a bit of effort went into it. Perhaps it was the shift to the Site Editor and the fact that more themes are depending on it. (e.g. there's no "home page" to land in for those themes)

@twstokes
Copy link
Contributor Author

twstokes commented Mar 29, 2024

AztecVerificationPromptHelper is an interesting one... what I did to test:

  1. Create a new account on WordPress.com web by entering my email address
  2. Do not verify the email address (Dotcom web will show a banner that the email address has not yet been verified)
  3. Set a password for the account
  4. Start a new draft on web using the "Classic" editor (so the app will use Aztec)
  5. Authenticate into the app with the account email and password from step 3
  6. Open the draft (which loaded Aztec), make edits, and tap "Publish"
  7. I expected the confirmation prompt, but it published successfully

I think this is due to the post being created on the web, so from the app the action is an update. I've also confirmed that if I duplicate the post that was created on the web inside the app, the verification prompt helper will show.

So it looks like we technically still have a need for it, albeit via an extreme edge case. Thoughts @kean? IMO if web lets users publish posts without confirming their email (which it does), maybe we should lift this from the app?

@kean
Copy link
Contributor

kean commented Mar 29, 2024

IMO if web lets users publish posts without confirming their email (which it does), maybe we should lift this from the app?

I agree. The risk is low – you'll presumably get an API error if there is a business rule that prevents that action.
The implementation has to go too. If it becomes a requirement, It should be based on the API response and not hardcoded in the app.

guard action == .publish 

It's not checking schedule 👀

@SiobhyB
Copy link
Contributor

SiobhyB commented Apr 3, 2024

If we agree that email verification shouldn't be necessary for publishing a post, then we could also fix #21472. :D I personally agree with matching the web, too.

@dcalhoun
Copy link
Member

dcalhoun commented Apr 3, 2024

I agree that removing email verification to mirror the web experience makes sense. The earliest references I could find for adding this mechanism are in 965a456 and 5367658, but it offers no explanation for why.

I noticed the logic appears to be referenced in the GutenbergViewController. Is there a scenario where the verification is applied there as well?

verificationPromptHelper = AztecVerificationPromptHelper(account: self.post.blog.account)

@twstokes
Copy link
Contributor Author

twstokes commented Apr 3, 2024

Is there a scenario where the verification is applied there as well?

That's correct @dcalhoun - we show it in the block editor.

@dcalhoun
Copy link
Member

dcalhoun commented Apr 3, 2024

My thought is that the email verification might've been a spam site deterrent, a tool to increase marketing contacts, a mechanism to help prevent account lockout, or all of those.

I still think it is odd for the web and mobile app experiences to diverge, but is it worth first asking if the current web experience is a bug?

@twstokes
Copy link
Contributor Author

twstokes commented Apr 4, 2024

That's a good thought @dcalhoun. After seeing the context below (see ref) it seems intentional, but it would be good to double-check the latest as it's got some age to it. I also confirmed that a site can be launched without confirming the address.

ref: p58i-bwV-p2

@twstokes
Copy link
Contributor Author

twstokes commented Apr 5, 2024

@kean I'm going to unassign myself from this issue, but please let me know if there's anything else to address. To summarize, it looks like:

Can be removed:

  • ✅ EditPageViewController(homepage:)
  • ✅ PostEditorAction.continueFromHomepageEditing

Can be removed if it's confirmed that the web's behavior is not a bug:

  • ⚠️ AztecVerificationPromptHelper

We could also proceed with the same treatment for Mobile Gutenberg and resolve this issue as pointed out by @SiobhyB.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants