-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Rename Twitter/X workflow task to match updated view names #15930
Rename Twitter/X workflow task to match updated view names #15930
Conversation
…hape runtime error
WalkthroughWalkthroughThe updates across various files in the OrchardCore.Twitter module involve renaming the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
Thank you for submitting your first pull request, awesome! 🚀 If you haven't already, please take a moment to review our contribution guide. This guide provides helpful information to ensure your contribution aligns with our standards. A core team member will review your pull request. If you like Orchard Core, please star our repo and join our community channels |
@dotnet-policy-service agree |
Thanks, @davidpuplava for opening this PR, IMHO we either revert the task rename or create a migration to avoid breaking existing workflows, please create a database migration and let me know if you need help on this |
That's the right thing to do. I'll try and add a database migration to do that and will let you know if I have any questions. |
Yeah, this rename is not showing all the things it can break. I would revert everything IMO. As long as X doesn't sue us for using the Twitter name ;) We could also clone the module to use the X language and deprecate the other one, while providing an opt-in migration path to the new module until a version when we definitely remove it. Users could even do it manually at least since both would exist. |
@hishamco I added a Data Migration but it doesn't seem to run even though I added it to Startup. Looks like the
Thanks for pointing this out @sebastienros. I'll revise this PR to change the view names back. |
Please update the PR to show why the migration is not working, which might be a bug that needs a fix, otherwise, we will revert the rename change |
…rch and Users modules
False alarm. I have the migrations working now. I overlooked that migration was already applied while testing. I've pushed up a data migration that renames the task in any workflow type definitions. I tested on my existing tenant that had workflows created prior to rebranding. I confirmed that instances do not need a rename because they refer back to the workflow type by id. I tested a blocking workflow that was in progress and that too does not seem to be a problem as @Piedone pointed out while answering my question about how to migrate. Do recipes and deployments need to be considered for migrating? I see that there is an IRecipeMigrator concept but I think that is more for recipes that are packaged with a module. EDIT I should point out that I modeled the migrations after similar code I found in the Lucene Search module rename here, and Orchard Users module rename here. Admittedly, I don't know what ShellScope.DeferredTask() does or if it appropriate here, but I thought best to match how other modules migrated content items in the document table. |
src/OrchardCore.Modules/OrchardCore.Twitter/Migrations/TwitterMigrations.cs
Outdated
Show resolved
Hide resolved
…arning as error build failure
src/OrchardCore.Modules/OrchardCore.Twitter/Workflows/Activities/UpdateXStatusTask.cs
Outdated
Show resolved
Hide resolved
src/OrchardCore.Modules/OrchardCore.Twitter/Migrations/TwitterMigrations.cs
Outdated
Show resolved
Hide resolved
src/OrchardCore.Modules/OrchardCore.Twitter/Migrations/TwitterMigrations.cs
Outdated
Show resolved
Hide resolved
src/OrchardCore.Modules/OrchardCore.Twitter/Migrations/TwitterMigrations.cs
Outdated
Show resolved
Hide resolved
src/OrchardCore.Modules/OrchardCore.Twitter/Workflows/Activities/UpdateXStatusTask.cs
Outdated
Show resolved
Hide resolved
This pull request has merge conflicts. Please resolve those before requesting a review. |
@davidpuplava for instructions on how to resolve the merge conflicts due to #16572 please follow the step listed in this comment. |
This pull request has merge conflicts. Please resolve those before requesting a review. |
…-x-status-task-shape-not-found
I've tested against the X/Twitter v1.1 API and confirm the But...I'm not certain that changing the workflow task type name ( Changing the UI strings to "X (Twitter)" is beneficial for the user. Changing type names benefits developers, but the single letter company name X is ambiguous enough that "XTwitter" is more descriptive anyway. So the question stands, is the risk of changing the type name worth the value gained in simply adding and 'X' in front of Twitter. I originally opened this PR only to get past the unhandled exception that was ultimately fixed with #16350 by changing the type names back. That said, it's worth considering if this PR should be closed without merging. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checking this out again, I agree. My head hurts due to all the wasted effort to adapt to such a pointless rebranding, but I'd keep every technical identifier (file names, types, HTML IDs...) as they were (#16350 reverted the shape template renames already). We shouldn't have done anything else than update UI labels. So, please in this PR:
- Only keep the UI string renames (but do keep all of those), having "(Twitter)" everywhere makes it possible to search for things.
- Even revert the
AdminMenuSignin
class/ID "X" rename, including inNavigationItemText-x.Id.cshtml
, but do add "(Twitter)" here too. - Check the module for other cases of these and do the same.
If you @hishamco @sebastienros object to this please comment.
Zoltan I think we did similar things in both Azure AD & Facebook Meta Let me review this one more time and processed forward |
Yes, and that was a waste of time as well. I'm complaining about the companies doing the rebranding, i.e. Twitter in this case. |
Hopefully this is not a matter for Lombiq one day :) |
Yes, looks like UI strings were updated to use the Meta and EntraID names, but the type names were left as is, mostly. I'll work towards @Piedone instructions to put the type names back and focus on the UI Strings. My interest in the X/Twitter module is because I implemented the v2 endpoint for posting to X. I'll submit that as a PR after I take care of this one. The v2 API for X is substantial enough that I could clone the Twitter module and make a new |
…gration since workflow task is no longer changed
…-x-status-task-shape-not-found
This PR is ready for review. I changed type/class/id names back to their original names prior to the rebrand based on what I could find in #15528 but I left the changes to the documentation pages. I tested both workflows for both new and old Update Twitter Status tasks, and confirm they hit the v1.1 API for X/Twitter. |
…isted to update workflow task names in Documents table
src/OrchardCore.Modules/OrchardCore.Twitter/Views/TwitterSettings.Edit.cshtml
Outdated
Show resolved
Hide resolved
src/OrchardCore.Modules/OrchardCore.Twitter/Workflows/Startup.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Zoltán Lehóczky <[email protected]>
Co-authored-by: Zoltán Lehóczky <[email protected]>
…-x-status-task-shape-not-found
Would you like to also review this, @hishamco? |
I will do a quick one now |
This PR fixes the following error
Exception: The shape type 'UpdateTwitterStatusTask_Fields_Thumbnail' is not found
and similar exceptions when editing a workflow while theX Integration
feature is enabled.Steps to Reproduce
main
branch or any branch that contains commit9e03d4f85267762e5446b42ad16d281e06f61f41
test
(or whatever name)Analysis
Looks to be related to the this PR to rebrand Twitter to X. The view files for the Workflow Activity Task were renamed but the task itself was not renamed.
Solution
After I renamed the Task to match the view names, the exception disappears and I can add the Update Status task a workflow.
Considerations
UpdateTwitterStatusTask
toUpdateXStatusTask
breaks existing Workflows that were created with the originalUpdateTwitterStatusTask
name. The workaround is to manually edit the database JSON to update the workflow definition to the new task name. Ideally, this change would happen automatically but I'm not sure how. This is why I opened a discussion on whether a DataMigration can be used to achieve this. In all honestly though, this Twitter/X may not be used enough to try and solve this automatically, especially since editing the database entry was trivial.Fixes #16349
Summary by CodeRabbit
UpdateTwitterStatusTask
toUpdateXStatusTask
across various components in the OrchardCore.Twitter module to reflect updated functionality.