Rename Twitter/X workflow task to match updated view names #15930
Conversation
…hape runtime error
WalkthroughThe 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 Configration 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. |
| await using var connection = dbConnectionAccessor.CreateConnection(); | ||
| await connection.OpenAsync(); | ||
| await using var transaction = await connection.BeginTransactionAsync(session.Store.Configuration.IsolationLevel); | ||
| var dialect = session.Store.Configuration.SqlDialect; |
There was a problem hiding this comment.
I think this is wrong, no need to rely on specific data storage
There was a problem hiding this comment.
I think this is wrong, no need to rely on specific data storage
What do you mean? I think those variables are all using interfaces from System.Data.Common to avoid relying on a specific database implementation. Is there different way to update the Workflow item types aside from updating the database directly?
There was a problem hiding this comment.
Ignore my above comment, seems I conflict with other PR :)
…arning as error build failure
This PR fixes the following error
Exception: The shape type 'UpdateTwitterStatusTask_Fields_Thumbnail' is not foundand similar exceptions when editing a workflow while theX Integrationfeature is enabled.Steps to Reproduce
mainbranch or any branch that contains commit9e03d4f85267762e5446b42ad16d281e06f61f41test(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
UpdateTwitterStatusTasktoUpdateXStatusTaskbreaks existing Workflows that were created with the originalUpdateTwitterStatusTaskname. 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.Summary by CodeRabbit
UpdateTwitterStatusTasktoUpdateXStatusTaskacross various components in the OrchardCore.Twitter module to reflect updated functionality.