? ? Failure

User tests: Successful: Unsuccessful:

avatar PhilETaylor
PhilETaylor
1 May 2021

#33466 @bembelimen @richard67 @joomdonation

Fulfils these new requirements from others

  • Works with Categories and Tags which need to extend the query
  • Allow calling the method in any order
  • Allow calling the method as many times as you like
  • Don't change $this->query directly, clone it.
  • Abit easier to extend/change the query to meet the need of child model class.
    - Resets the user state, so clicking on the Call To Action (Add/New) button doesn't start a new item in the Trash
avatar PhilETaylor PhilETaylor - open - 1 May 2021
avatar PhilETaylor PhilETaylor - change - 1 May 2021
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 1 May 2021
Category Administration com_categories com_tags Libraries
avatar PhilETaylor PhilETaylor - change - 1 May 2021
The description was changed
avatar PhilETaylor PhilETaylor - edited - 1 May 2021
avatar PhilETaylor PhilETaylor - change - 1 May 2021
The description was changed
avatar PhilETaylor PhilETaylor - edited - 1 May 2021
avatar PhilETaylor PhilETaylor - change - 1 May 2021
The description was changed
avatar PhilETaylor PhilETaylor - edited - 1 May 2021
avatar PhilETaylor PhilETaylor - change - 1 May 2021
The description was changed
avatar PhilETaylor PhilETaylor - edited - 1 May 2021
avatar PhilETaylor PhilETaylor - change - 1 May 2021
The description was changed
avatar PhilETaylor PhilETaylor - edited - 1 May 2021
avatar PhilETaylor PhilETaylor - change - 1 May 2021
Labels Added: ?
avatar bembelimen
bembelimen - comment - 1 May 2021

Thank you for the improvements.

avatar PhilETaylor
PhilETaylor - comment - 1 May 2021

But do they work :)

avatar richard67
richard67 - comment - 1 May 2021

Adding release blocker label as inherited from the issue.

avatar brianteeman
brianteeman - comment - 1 May 2021

This isnt a release blocker. If this matches the criteria to block a release then the tag should be applied to 90% of pr

avatar richard67
richard67 - comment - 1 May 2021

Well I don't really care. It will be merged in time anyway if it's good. Will see if I can test tomorrow.

avatar joomdonation
joomdonation - comment - 2 May 2021

Could you please move reset states logic to a separate PR? My feeling is that getIsEmptyState should not do the reset states itself. It is up to caller to do that if it wants. Further more, I'm unsure if the implementation of resetState is right but that is a different discussion

For this one, could we please focus on fixing the empty state logic? One problem at a time.

avatar PhilETaylor
PhilETaylor - comment - 2 May 2021

So once again one person wants it and another person doesn't want it. I just can't win. It works, And has zero knock on effects.

avatar joomdonation
joomdonation - comment - 2 May 2021

So once again one person wants it and another person doesn't want it. I just can't win. It works, And has zero knock on effects.

I just express my concern and want to move it to separate PR so that maintainers and review and make decision easier. At the end, you don't need to make everyone happy. If you think it is OK, just leave it in this PR and maintainers will decide.

avatar joomdonation
joomdonation - comment - 2 May 2021

So I still believe (myself) that resetState should be implemented like that or is called within getIsEmptyState (maybe it is just me, don't worry). As I said, you don't have to make everyone happy. If you want to keep it, leave it as how it is. Other maintainers will review and make decision.

In case you agree with me, I made a PR to your branch PhilETaylor#7 which I think it will make this PR safe to merge. Please take a look at it (especially the change in Categories model which we discussed earlier in this PR) and if it works and you are happy, merge it.

avatar richard67
richard67 - comment - 2 May 2021

For me it's ok to remove the resetState here and do it with another PR, to come forward here. I don't even insist on my issue to be fixed, I just thought it could be a good idea.

avatar PhilETaylor
PhilETaylor - comment - 2 May 2021

I have just landed and will work on this today. Again. But the pr proposed by @joomdonation i think has a major flaw - but I will be verbose in explaining that when I'm next at a desk and not sat at the controls of an airplane flying

avatar PhilETaylor PhilETaylor - change - 2 May 2021
The description was changed
avatar PhilETaylor PhilETaylor - edited - 2 May 2021
avatar PhilETaylor
PhilETaylor - comment - 2 May 2021

I take it back. The concern I had was unfounded as using _getListCount takes my concern (about joined tables like tags still being left in the select, when they were removed as a join). _getListCount does this clearing of select and readds select(*) for us,

I have merged your PR to my branch.

avatar PhilETaylor
PhilETaylor - comment - 2 May 2021

I have removed limit and offset from being cleared in getEmptyStateQuery - these are removed by _getListCount anyway

avatar PhilETaylor
PhilETaylor - comment - 2 May 2021

Once this is merged I'll do a quick recap of all the issues and outstanding states in #33266 and then I'll be stepping back and taking a break. Thanks for your support on these, I think Joomla is better for this new feature and only sad the rest of my work has not made it as far.

avatar joomdonation joomdonation - test_item - 2 May 2021 - Tested successfully
avatar joomdonation
joomdonation - comment - 2 May 2021

I have tested this item successfully on 66b2c1c


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/33471.

avatar joomdonation
joomdonation - comment - 2 May 2021

@PhilETaylor Thanks for working so hard for Joomla! these days.

avatar richard67 richard67 - alter_testresult - 2 May 2021 - joomdonation: Tested successfully
avatar richard67
richard67 - comment - 2 May 2021

I've updated the branch to latest 4.0-dev to get rid of the unrelated javascript-cs error in drone and restored the previous test result in the issue tracker.

avatar drmenzelit drmenzelit - test_item - 2 May 2021 - Tested successfully
avatar drmenzelit
drmenzelit - comment - 2 May 2021

I have tested this item successfully on af0ddad

I tested all views with empty states and checked using search and filters. I couldn't find problems. And the problem with "disappearing" categories when using filter tag is gone.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/33471.

avatar richard67 richard67 - change - 2 May 2021
Status Pending Ready to Commit
avatar richard67
richard67 - comment - 2 May 2021

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/33471.

avatar bembelimen bembelimen - change - 2 May 2021
Status Ready to Commit Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2021-05-02 19:30:46
Closed_By bembelimen
Labels Added: ?
avatar bembelimen bembelimen - close - 2 May 2021
avatar bembelimen bembelimen - merge - 2 May 2021
avatar bembelimen
bembelimen - comment - 2 May 2021

Thank you!

Add a Comment

Login with GitHub to post a comment