CMSMain - Fixed and enhanced BatchActionParameters

[mooror]

Description

This is the first of two PRs (the second will be in the admin module) for implementing the enhancement described in issue #2744

Changes

• Fixed the $batchActions variable by making it use the registeredActions() method
• Made the loop use FieldHolder instead of Field so CMS fields display correctly
• Added hidden styles to the batch action div for jQuery show/hide functionality (found in Admin module PR)

[michalkleiner]

Glanced over it and it seems ok, I suspect the admin PR needs to be merged first for the display:none styles to work? I guess we don't want to hide the fields randomly without the admin changes. Do we need to add a minimum version constraint to composer.json to ensure the dependencies work together correctly? Or is there a way to make it safe without the admin PR?

[mooror]

Hey Michal @michalkleiner, thanks for giving this a look over. I appreciate your time.

As for your question, as far as I'm aware the BatchActionParameters() method that I change in this PR isn't called anywhere in Silverstripe's core code (strangely enough). I did a few searches to see if I could find any references to it in php or ss files, and couldn't find any. I shared in #2744 that I suspected this was a feature that was never (re)implemented.

If I'm correct (and didn't just miss the references somehow) then this PR should (theoretically?) be safe as is.

[mooror]

That being said, I almost have the second PR complete, so we can wait until after that one's done

[GuySartorelli]

Just a few changes from a linting POV.
As a whole it looks good, though I haven't tried it locally yet. Will wait for the other PR but I share the same concerns as @michalkleiner

[mooror]

@GuySartorelli I made the changes you asked for.

Unfortunately I'm not very familiar with the pull request process. I see that the changes are listed, but do I need to do anything else to incorporate them into the pull request?

[GuySartorelli]

Nope, you've done it correctly. As soon as you pushed the changes to your branch, they became part of the PR.
Theoretically before this gets merged it would be nice for the commits to be squashed, but we can do that at merge-time so it's not a big deal.

[mooror]

Oh okay, perfect 👍 . As for the squash, I've honestly never done one before 😅, but if you need it done I would be willing to learn.

[michalkleiner]

@mooror the only concern (from the cross-repo PRs and composer constraints perspective) is basically adding the display:none to the field. Would that have an impact if the other PR wasn't created? Could we e.g. only add it to a second and subsequent field so it won't have any impact for the current state of things?

[mooror]

the only concern (from the cross-repo PRs and composer constraints perspective) is basically adding the display:none to the field. Would that have an impact if the other PR wasn't created?

I think I understand where your coming from Michal, as even if there was no existing code that calls the BatchActionParameters() method, adding display hidden could make it harder to work with for people using it in the future. One thing I want to point out though is that the returned LiteralField already had display:none on it before I changed anything in my PR. Here's the original line:

return new LiteralField("BatchActionParameters", '<div id="BatchActionParameters" style="display:none">'.$pageHtml.'</div>');

Even without my PR, anyone who would use the method in the future would have to write some code (presumably javascript code) to remove the "display:none" and display the field.

So to my mind, the addition of the second "display:none" on the inner div tag (which I added in my PR) doesn't change much in the sense that the field was hidden before my PR, and hidden after my PR, though it is true that anyone who uses the code would need to write an extra line or two of javascript to unhide the div as well.

If you guys think it will be a big issue, I can try to rewrite the javascript to automatically add "display:none" at execution time instead. I just thought this way would be more performant

[michalkleiner]

Ah, ok, that's the piece I was missing (or overlooked) that the method wasn't called from anywhere before. In that case it's probably safe(-ish) to merge this PR first. There might be folks who used it as it's a public method but since we're merging it to 4 it's an enhancement and would not be released anytime soon.

I'm giving it a ✅ from the code changes perspective but would defer to @GuySartorelli on the mergeability/destination branch for the PR.

[GuySartorelli]

I agree, given that it's effectively dead code without this PR the risk is pretty low. I have no problems with it as an enhancement targetting 4.
I'll try get around to testing it sometime this week.

Those changes have been made

[mooror]

@michalkleiner Ah okay, that makes sense, and you're right that it's possible someone may have call the method in their module. Thinking about it now, I suppose the chances of that are pretty low as it wasn't even documented, but then again if I found it someone else attempting the same thing may have used it to created a work around. Hadn't thought of that

since we're merging it to 4 it's an enhancement and would not be released anytime soon

Also, I'm not vary familiar with Silverstripe's branch structure and release cycle, so when I submitted the PR, I assumed that the 4 branch was the working branch that silverstripe 4.X branches would be created from (meaning this change would theoretically be added when 4.12 was created?). But now I'm wondering if I was mistaken in that, so to clarify, what is the 4 branch used for?

[mooror]

I agree, given that it's effectively dead code without this PR the risk is pretty low

Yeah without the changes to the $batchActions variable, the method didn't even seem to work when called, which would make it pretty dead. But to be fair, while this PR might make the method usable, the method still won't be used anywhere without the second PR being merged into admin

I'll try get around to testing it sometime this week.

Thank you @GuySartorelli 🎩 Oh and will you be testing this PR on it's own, or in addition to the second one?

[GuySartorelli]

I assumed that the 4 branch was the working branch that silverstripe 4.X branches would be created from (meaning this change would theoretically be added when 4.12 was created?). But now I'm wondering if I was mistaken in that, so to clarify, what is the 4 branch used for?

You're correct, the 4 branch is the feature development branch. So when we start the release process for the next minor version, a new 4.12 branch will be made from 4, and will be the basis for that release.
If this was a bug fix, it would make sense to target 4.11 or (for critical bugs) 4.10 so that we could include it in a patch release.

will you be testing this PR on it's own, or in addition to the second one?

If this PR doesn't work without the second one (which it sounds like is the case), I'll test them together.

[GuySartorelli]

Looks good, works locally. Without the accompanying PR there are no issues so the composer constraint doesn't need to be modified.
Good work! Happy to merge this.

If you're feeling adventurous, it would be awesome if you can write some tests (PR against this repository) and docs (PR against https://github.com/silverstripe/developer-docs) which I should have asked for earlier. :p

[mooror]

Without the accompanying PR there are no issues so the composer constraint doesn't need to be modified.

Glad to hear it 🚀
When I get some free time, I'll try to write up that documentation, I would have to do that for our company anyway as I'll probably forget how it all works a few months down the road 😅. As for the test writing, I've never done that before, but I need to learn how to do that any way, so I might also do that as well

[GuySartorelli]

Awesome, thank you. If you need any help writing tests feel free to @ me.

[GuySartorelli]

@mooror Heya, just wondering if you've been able to make time to write some documentation and tests for this?

[mooror]

Hey Guy @GuySartorelli, sorry for the late reply. Things have been extra busy as of late and I haven't been on github very often

To answer your question, I haven't done either. Realistically I think I'll be able to write up some documentation, but I'm not so sure about the unit tests, as I still have to learn how to write those, and at the moment I don't know if I'll have the time to invest in that (though I really need to learn those at some point)

[GuySartorelli]

No worries, even the documentation would be a great help if you can make time for it. One less thing I need to worry about.