Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature presidecms 1132 #443

Closed

Conversation

@cfmitrah
Copy link
Contributor

@cfmitrah cfmitrah commented Mar 28, 2018

No description provided.

@cfmitrah
Copy link
Contributor Author

@cfmitrah cfmitrah commented Apr 17, 2018

@DominicWatson @sebduggan Did you get some time to review this feature PR?

Loading

@sebduggan
Copy link
Contributor

@sebduggan sebduggan commented Apr 17, 2018

Not had time to look at it yet, but will do soon...

Loading

@cfmitrah
Copy link
Contributor Author

@cfmitrah cfmitrah commented Apr 17, 2018

Sooner is better to avoid merge conflicts :)

Loading

Copy link
Contributor

@sebduggan sebduggan left a comment

The file views/admin/datamanager/groupVersionNavigator.cfm seems to be missing...

Loading

@cfmitrah
Copy link
Contributor Author

@cfmitrah cfmitrah commented Apr 19, 2018

@sebduggan We've added the missing file views/admin/datamanager/groupVersionNavigator.cfm
Please check that.

Thank you

Loading

Copy link
Contributor

@DominicWatson DominicWatson left a comment

Hey guys, this looks great (have tried it out locally). I have a few review comments though about the code.

Would be great to go through those and ammend before we merge.

Great effort though, thank you!

Loading

@@ -1035,6 +1035,53 @@ component extends="preside.system.base.AdminHandler" {
return renderView( view="admin/datamanager/versionNavigator", args=args );
}

private string function groupVersionNavigator( event, rc, prc, args={} ) {
Copy link
Contributor

@DominicWatson DominicWatson Apr 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not think that this belongs in DataManager.cfc. It seems completely specific to system config so should go int he sysConfig admin handler I think.

Loading

var objectName = args.object ?: "";
var id = args.id ?: "";
var setting = structKeyList( event.getCollectionForForm( "system-config.#args.id#" ) );
var maxVersion = presideObjectService.getNextVersionNumber();
Copy link
Contributor

@DominicWatson DominicWatson Apr 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be avoided. This will grow the version table unnecessarily. Instead of this (and the query of queries below), we should add a method to the sysConfig service that calculates the max version for a category. This should be achievable with a single regular selectData query I should think. Rough pseudocode:

var version = $getPresideObject( versionObjectNameForSysConfgTable ).selectData(
      selectFields = [ "Max( _version_number ) as max_version_number" ]
    , filter = { category = arguments.category }
);

return Val( version.max_version_number );

Loading

public void function getConfigHistoryForAjaxDataTables( event, rc, prc ) {
prc.setting = structKeyList( event.getCollectionForForm( "system-config.#rc.id#" ) );

var nextVersionNumber = presideObjectService.getNextVersionNumber();
Copy link
Contributor

@DominicWatson DominicWatson Apr 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See other review comment about avoiding this. This method call inserts a new version number in the version number table. Instead, we shoud calculate the real max version number for the settings category.

Loading

@@ -1260,6 +1261,10 @@ component displayName="Preside Object Service" {
args.filterParams = { "#idField#" = arguments.id, _version_changed_fields = "%,#args.fieldName#,%" };
args.delete( "fieldName" );
args.delete( "id" );
} else if ( listLen( arguments.id ) GT 1 ){
Copy link
Contributor

@DominicWatson DominicWatson Apr 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we remove this? This is an unrelated change. You can achieve the same thing with:

selectData( filter={ id=arrayOfIds } );

This could be a useful approach, but I'd rather make that a separate change and rather use an array than a string list to avoid ambiguity.

Loading

Copy link
Contributor Author

@cfmitrah cfmitrah May 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DominicWatson
This is the line called and send configIds,
\system\handlers\admin\SysConfig.cfc : 193, Here if I implement the above changes means I've to change the dataManager and dataManagerService.cfc. Or else we've create a new service which we've to achieve this changes.
BTW Not sure whether we need to implement this change.

Loading

@@ -0,0 +1,62 @@
<cfscript>
Copy link
Contributor

@DominicWatson DominicWatson Apr 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be renamed and moved to sys config views folder (see previous comment on the DataManager.cfc)

Loading

@sebduggan
Copy link
Contributor

@sebduggan sebduggan commented Apr 20, 2018

One other small code style point: when writing cfscript, it's preferable to use <, >, ==, != etc, rather than LT, GT, EQ, NEQ...

Loading

@DominicWatson
Copy link
Contributor

@DominicWatson DominicWatson commented May 23, 2018

Hey @cfmitrah - thanks for the changes. I'm going to make a call to leave this out of 10.9. When testing this there were quite a few things not quite working right and as its complex I'd like us to spend the time to get it right before rushing into release.

Will provide more feedback after release.

Loading

@DominicWatson
Copy link
Contributor

@DominicWatson DominicWatson commented Jun 15, 2018

Garrrr, this got auto closed because the target branch has disappeared. Really sorry for this. You should be able to re-open from your branch though by targeting the stable branch.

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants