[4.0] Correct assets name for workflow state -> stage and ID #35102
Conversation
sanderpotjer
changed the title
|
This will also need update sql statements |
|
@brianteeman you're welcome. Update SQL statements to correct installation data which did not released stable version yet? But statements added anyway. |
@sanderpotjer Since we are in beta phase we grant updates working between the pre-releases (beta, rc and then to stable) and this requires the right update SQL scripts. |
|
@richard67 thanks for the explanation 👍 |
administrator/components/com_admin/sql/updates/mysql/4.0.0-2021-08-13.sql
Outdated
Show resolved
Hide resolved
administrator/components/com_admin/sql/updates/postgresql/4.0.0-2021-08-13.sql
Outdated
Show resolved
Hide resolved
administrator/components/com_admin/sql/updates/postgresql/4.0.0-2021-08-13.sql
Outdated
Show resolved
Hide resolved
|
@richard67 thanks for the feedback. Made the requested changes. Side note: I suggest to reconsider to support updates between non-stable versions, and recommend people to use a fresh install for stable versions instead 😉 |
…0-2021-08-13.sql Co-authored-by: Richard Fath <richard67@users.noreply.github.com>
Since we made the change it has made a massive difference, for the better, with people testing pull requests. Its slightly more work for the person creating the PR but its been worth it. |
|
Transitions an their records in assets are not correct (fresh installation) Transitions Assets |
| WHERE `name` = 'com_content.state.1'; | ||
|
|
||
| UPDATE `#__workflow_stages` | ||
| SET `asset_id` = (SELECT MAX(`id`) FROM `#__assets` WHERE `name` = 'com_content.stage.1') |
There was a problem hiding this comment.
I just came to this again and think I've missed something with my previous review:
- If someone has deleted the asset, the subquery will not return any record, and that will result in an SQL error here. It needs to change the subquery to a join so we don't need the same subquery again in the where clause for checking if there is some result or not.
- The where clause should be extended so it does not catch all stages with an asset ID of zero but only com_content's basic stage.
There was a problem hiding this comment.
Another issue with this UPDATE statement: If someone just opens and saves the workflow stage in backend on a current 4.0-dev without this PR applied, a new asset with the right name is created. So we should do this update only if there is no asset with the right name yet, otherwise we will have two of them.
|
@sanderpotjer The findings mentioned by @chmst above are not caused by your PR. She knows that. But it could make sense to fix it with your PR, too. The update SQL could become a bit complicated for that. I'll try to help with that as soon as I know if you intend to fix these findings here, too, or if not. Or if you are too busy or it becomes too complicated for you, I can take over and make a new PR for all. Just let me know what you prefer. |
| @@ -0,0 +1,8 @@ | |||
| -- after 4.0.0 RC6 | |||
There was a problem hiding this comment.
| -- after 4.0.0 RC6 |
We don't need these comments here.
Previous update SQL scripts have such because they were combined from several previously present update SQL scripts when we had combined them together in order to reduce their number. But here this is not the case.
| @@ -0,0 +1,8 @@ | |||
| -- after 4.0.0 RC6 | |||
There was a problem hiding this comment.
| -- after 4.0.0 RC6 |
We don't need these comments here.
Previous update SQL scripts have such because they were combined from several previously present update SQL scripts when we had combined them together in order to reduce their number. But here this is not the case.
|
@sanderpotjer Because it's better to explain with code changes than words what all would be necessary to fix the things mentioned in my review comments and in @chmst 's findings, I've created the draft PR #35113 which would replace this one here. Please let me know soon what you prefer: Shall I make that PR against your branch of this PR, so you will continue with your PR and update testing instructions? Or shall I take over, i.e. finish my draft PR and you close this one in favour of mine? Thanks in advance for your reply. |
|
@sanderpotjer Because I haven't got any feedback from you, I've decided to finish my PR #35113 , which replaces this one here and fixes the other issue mentioned in @chmst 's comment. I hope you are not angry. I'd be happy if you could help with testing. Thanks in advance, and thanks for reporting the issue and making this PR which was a good start. |
|
Closing in favour of #35113 . Please test. Thanks in advance. |
|
@richard67 I'm not angry. But if I am disappointed? Yes. A bit more time to respond on the weekends would be nice, or at least fork the PR I was working to remain my initial commits (and in that way credits). I respect and appreciate your and everyone else's work on Joomla, but this pull request does not motivate me to contribute further. |
|
@sanderpotjer I am sure that @richard67 acted in all good faith and that you appreciate that with just 48 hours until release some of the normal timelines have to be shortened. Of course it would have helped if you had been testing your extension (and thus using this one contribution as a marketing tool) much earlier and not at the last possible moment. This bug has been present for a very long time. |
|
@brianteeman dear co-founder of Joomla, what a sad comment and preconception. |
@sanderpotjer You are right, I should have done it that way. I only can say sorry for my mistake. I'll see if I can fix that. |
|
@richard67 Please don't spend more of your time to fix that, for similar situations in the future it would be nice 😉 |
|
@sanderpotjer Ok, thanks. It was like Brian said, I was driven by the tight release time schedule and the experience that it is hard and takes some time to get testers for such things and the assumption that you might not be available because of weekend. |
Correction assets installation data. Asset
namefor sample stage is currently incorrect (com_content.state.1instead ofcom_content.stage.1) and defaultasset_idfor#__workflow_stagesshould not be related to 0 but the correct asset.