magento-lts
magento-lts copied to clipboard
OpenMage
Added updated_at column to "System -> Cache Management" grid
Description (*)
Added column Refreshed At to caches grid.
Contribution checklist (*)
- [x] Pull request has a meaningful description of its purpose
- [x] All commits are accompanied by meaningful commit messages
- [x] All automated tests passed successfully (all builds are green)
- [x] Add yourself to contributors list
The proposed change is welcome.
What I would like to discuss is the place where the new column should be inserted. If we look at System > Index Management grid, it is placed to the left of the "Actions" column. As this column does not exist in "Cache Store Management", there are two options, before the Status column or after it. I would go with the option after, being the last column in the grid. This layout will follow the on in the "Index Management" grid.
There would be another comment. We are talking about a status update for a cache and not a refresh. The column name would be more appropriate as Updated At.
@ADDISON74 Moved column at the end of grid and added formating of date same as in index grid.
Im not against it, but columns can be added via event/observer. Whats the benefit - either cache is up2date or it is not?
Btw ... there are some generic events dispatched, that can be improved ... see #249 (and related)
1 - Positioning the column to the right is fine.
2 - The initial wording, in which the name of the column was "Refreshed At", it was not appropriate. "Update At" is found through grids, but also "Performed At". This is from Import/Export Profile > Profile History tab.
I would not rush to approve this PR because there are issues that need to be fixed.
1 - If the [Flash & Apply Updates] and [Flash Cache Storage] buttons are pressed on the "Update At" column, no data appears. The column remains empty, see below
2 - Test the 3 actions, Enable / Disable / Refresh. It would be natural for each of them that after pressing the [Submit] button the time of the event should be changed on the "Update At" column. Here are the results
Disable and Refresh = it is considered an update, the time changes.
Enable = it is not considered an update, time remains unchanged. There is problem for this action.
I've added the support for the "flush" actions, but https://github.com/OpenMage/magento-lts/pull/3739#discussion_r1483386101 still needs an answer, @Sekiphp can you check it out?
I confirm that the problem is solved for the both [Flush] buttons.
What do we do with the Enable action?
For example, one of the cache types is Disable. Let's select that row, then choose the Enable action from the dropdown list and press the [Submit] button. It would be natural to see an update in time, but the previous one remained. See the result below
BEFORE (Disable action)
AFTER (Enable action)
It is still a problem because any status update for a cache type, no matter it is Disable / Refresh / Enable must appear in the grid. As it is now the Enable action is not considered an update.
The last change created several problems. I have identified three of them.
1 - By pressing the [Flush & Apply Updates] button, for the first type of cache in the grid called "Configuration" it disappears the time on the "Update At" column. The [Flush Cache Storage] button does not have this problem.
2 - If I select a type of cache that has the Enable status and apply the Disable action, nothing is displayed on the "Update At" column. Here it is debatable, because if someone needs to know when Disable ran, he won't know.
3 - If I select a type of cache that has the Enable status and apply the Enable action to it, the time remains the same on the "Update At" column. Here it is debatable too, because a new Enable action over another is still an action and the time should be updated.
- weird, i've to check
- IMHO it's correct that no updated_at is shown if a cache is disabled.
- also it's correct if updated_at doesn't update if you enable a cache that's already enabled (it doesn't get refreshed in that case)
IMHO when disabling and re-enabling the cache, the datetime should be updated as it is equivalent to flushing the cache
But I'm thinking that this data is misleading because caches can be partially updated on various occasions, for example when saving a product. Is the date updated in this case? It would change frequently, which makes it pointless to keep track of it.
when enabled yes for sure, when enabling an enabled one it doesn't get cleaned (AFAIK) so the updated_at shouldn't change.
when a cache gets partially updated the value here doesn't change, it only change for a complete refresh
when enabled yes for sure, when enabling an enabled one it doesn't get cleaned (AFAIK) so the updated_at shouldn't change.
when you disable and then enable the cache not start to zero?
when a cache gets partially updated the value here doesn't change, it only change for a complete refresh
I'm starting to think that this information is useless and misleading PS: I don't know how to remove my approval/review.
when enabled yes for sure, when enabling an enabled one it doesn't get cleaned (AFAIK) so the updated_at shouldn't change.
when you disable and then enable the cache not start to zero?
yes, I said that if you enable a disabled cache then the info is correctly shown.
- IMHO it's correct that no updated_at is shown if a cache is disabled.
- also it's correct if updated_at doesn't update if you enable a cache that's already enabled (it doesn't get refreshed in that case)
The topic is worth discussing:
2 - Would I be interested in when was the last time that type of cache was updated? If yes, then the moment when it was disabled must be displayed.
3 - What happens at the ground level in the event of an Enable action on an already enabled cache type? Does anything change? If yes, then the time of the update must be shown.
- if it's disabled I'd leave it empty instead of saving the time of disabling, i guess it's personal preference
- afaik caches don't get reset/refreshed in enabled in case of being already enabled
btw since there's no agreement I'll leave the continuation to the original posted, otherwise let's close it, I don't want to continue working on something that brings so many arguments when the original author is not participating anymore.
The last change created several problems. I have identified three of them.
1 - By pressing the [Flush & Apply Updates] button, for the first type of cache in the grid called "Configuration" it disappears the time on the "Update At" column. The [Flush Cache Storage] button does not have this problem.
hehehe interesting find, when "flush and apply updates" the config cache gets disabled temporarily (in order to apply the updates) and that's why it shows empty. I want to work and fix this to leave this PR in a working state and then I'll leave it :-)
I should have fixed the last problem and I think the funcionality could now be merged, it's not the most necessary info but I think it's an interesting data to have (many times I've been ask "when do the cache get reset" and this way the users know without asking).
It doesn't cover the partial invalidation but I think that's not needed and not even the case for this type of info.
I also think we shouldn't always try to merge only perfect things, because many times we get stuck and do nothing. We should try not to add bugs but I still think that a 80%-done new feature is better than nothing.
I also think we shouldn't always try to merge only perfect things, because many times we get stuck and do nothing. We should try not to add bugs but I still think that a 80%-done new feature is better than nothing.
I agree, OpenMage needs to change to be considered as a viable alternative solution, we should focus on new essential features that are missing and other platforms have.
About this feature in my opinion it is useless but if you support me in others PR we can make an exchange 😂
I did all the possible combinations:
1 - I selected an Enabled cache type that does not have update information (nothing appears in a cell) and applied the Refresh action => OK.
2 - I selected an Enabled cache type that has no update information (nothing appears in the cell) and I applied the Enable action => nothing was updated, there is no time in the cell.
3 - I selected an Enabled cache type and I applied the Disable action. The information in the cell disappears => I remain of the opinion that I should know when I did the action. See bellow how ugly looks the grid without this information.
4 - I selected an Enabled cache type that has update information and I applied the Enable action => the time was not updated.
5 - I selected an Enabled cache type that has update information and I applied the Refresh action => OK.
I pressed the button [Flush & Apply Update] => OK, all cache types are updated, except the Disabled ones.
I pressed the button [Flush Cache Storage] => OK, all cache types are updated except the Disabled ones.
About this feature in my opinion it is useless but if you support me in others PR we can make an exchange 😂
@empiricompany sincerely this comment makes me sad. I review literally every PR and try to get everyone's opinion and always try to get everybody's work appreciated and merged. reading this hurts me. I literally takes other people's PR (like this one) not to leave them to die forever like it happened for years. this is not even my PR, I only added and fixed it because I wanted to help.
@fballiano - You don't have to take the comments personally. Even if I wasted an hour with this PR, it was useful because I analyzed situations. I appreciate you for the simple fact that you moved it step by step almost to the end, when the author just pushed it, then when the problems appeared he disappeared in the fog. I don't consider it final because I'm not reconciled to the idea of missing the update information if one or more caches are disabled.
I also think we shouldn't always try to merge only perfect things, because many times we get stuck and do nothing. We should try not to add bugs but I still think that a 80%-done new feature is better than nothing.
I agree, OpenMage needs to change to be considered as a viable alternative solution, we should focus on new essential features that are missing and other platforms have.
About this feature in my opinion it is useless but if you support me in others PR we can make an exchange 😂
@empiricompany sincerely this comment makes me sad. I review literally every PR and try to get everyone's opinion and always try to get everybody's work appreciated and merged. reading this hurts me. I literally takes other people's PR (like this one) not to leave them to die forever like it happened for years. this is not even my PR, I only added and fixed it because I wanted to help.
@fballiano, you misunderstood me, I didn't mean to offend. I really appreciate your work and I think that without you this project it would not be possible. You and so many others in this community are heroes to me. I just wanted to say that I think we should focus on other more important things, always in my opinion. I was wrong to say it here with a joke but I see a lot of participation on other PRs but that do not make us evolve. I hope you will accept my apology.
I would approve it, but I have a visual issue with the last column on the right when the time is not displayed. It looks ugly without anything. In addition, disabling a cache type is itself an update.
I re-checked this PR, in order to implement what @ADDISON74 was asking. sadly it's not possible to do that, I'll try to explain:
- the "updated_at" time is saved in the cache itself
- if a cache is disable I can't save anything into it, not even the updated_at
Although I like this PR and I spent quite some time fixing it, since the original author abandoned it and there's no consensus among admins, I'll close it.
I agree. Initially it seemed like a good idea to know when the cache was updated, but after analyzing it in detail we found out some issues. It is not worth spending time because the advantages of this feature are modest. Anyway, I will mark it with a label.
Above in a post @sreichel mentioned a more elegant solution if this feature is really wanted.