Options GUI #6

Open
wants to merge 28 commits into
from

Conversation

Projects
None yet
3 participants
@bchoquet-heliopsis
Contributor

bchoquet-heliopsis commented Apr 3, 2012

At last, a long time discussed pull request :)

I tried to make it as simple as possible :

  • each handler's options are defined in sqliimport.ini.append.php
  • on handler selection in admin interface, an AJAX request is made returning an HTML form for handler options
  • form is generated by loading a subtemplate for each option according to its type
  • each subtemplate is responsible for adding a form field named ImportOptions[<option name>] whose value will be stored in database

NB : there is no server side validation for options input but there wasn't either in the textarea version

For complex option input, one can define a YUI3 module to manage his needs and fill in a hidden input field with the computed value. A boilerplate for such a module can be seen in design/admin/javascript/sqliimportsampleoptionmodule.js.

Finally, I added a file option type enabling file upload, validation and storage.

I reckon the code is well commented enough, tell me if you need any more details

@lolautruche

This comment has been minimized.

Show comment Hide comment
@lolautruche

lolautruche Apr 3, 2012

Owner

Wow O_O ! This is what I call a pull request :-).
I'll need some time to review it...

Thanks !

Owner

lolautruche commented Apr 3, 2012

Wow O_O ! This is what I call a pull request :-).
I'll need some time to review it...

Thanks !

@lolautruche

This comment has been minimized.

Show comment Hide comment
@lolautruche

lolautruche Apr 4, 2012

Owner

First remark : Please add the licence header to every new file :)

Owner

lolautruche commented Apr 4, 2012

First remark : Please add the licence header to every new file :)

+ $csvOptions = new SQLICSVOptions( array() );
+
+ //extract headers in temp file to ensure fast csv parsing
+ $f = fopen( $filePath, 'r' );

This comment has been minimized.

Show comment Hide comment
@lolautruche

lolautruche Apr 4, 2012

Owner

Not cluster safe !

@lolautruche

lolautruche Apr 4, 2012

Owner

Not cluster safe !

This comment has been minimized.

Show comment Hide comment
@bchoquet-heliopsis

bchoquet-heliopsis Apr 13, 2012

Contributor

This method is used to validate file format before storing it on the server. Therefore it must not be cluster aware as it is used on temporary files.

However I'm updating the initialize() method to fetch file from cluster before calling this method.

@bchoquet-heliopsis

bchoquet-heliopsis Apr 13, 2012

Contributor

This method is used to validate file format before storing it on the server. Therefore it must not be cluster aware as it is used on temporary files.

However I'm updating the initialize() method to fetch file from cluster before calling this method.

This comment has been minimized.

Show comment Hide comment
@lolautruche

lolautruche Nov 28, 2013

Owner

OK

classes/sqliimportjsserverfunctions.php
+ $handler = $args[0];
+
+ //arg 2 may be a scheduled import id
+ if( count( $args ) > 1 ){

This comment has been minimized.

Show comment Hide comment
@lolautruche

lolautruche Apr 4, 2012

Owner

CS : Braces on a new line

@lolautruche

lolautruche Apr 4, 2012

Owner

CS : Braces on a new line

classes/sqliimportjsserverfunctions.php
+ if( count( $args ) > 1 ){
+ $scheduledImport = SQLIScheduledImport::fetch( $args[1] );
+
+ if( !$scheduledImport ){

This comment has been minimized.

Show comment Hide comment
@lolautruche

lolautruche Apr 4, 2012

Owner

CS : Braces on a new line

@lolautruche

lolautruche Apr 4, 2012

Owner

CS : Braces on a new line

classes/sqliimportjsserverfunctions.php
+ $handler . DIRECTORY_SEPARATOR .
+ $option;
+
+ $file->store( $dir );

This comment has been minimized.

Show comment Hide comment
@lolautruche

lolautruche Apr 4, 2012

Owner

This is not cluster safe. You should after that make use of eZClusterFileHandler in order to properly store the file in the cluster, and then remove the local file (still with eZClusterFileHandler)

@lolautruche

lolautruche Apr 4, 2012

Owner

This is not cluster safe. You should after that make use of eZClusterFileHandler in order to properly store the file in the cluster, and then remove the local file (still with eZClusterFileHandler)

@lolautruche

This comment has been minimized.

Show comment Hide comment
@lolautruche

lolautruche Apr 4, 2012

Owner

Could you please also add some documentation ? You can make a separate doc file if you wish (markdown format is preferred)

Owner

lolautruche commented Apr 4, 2012

Could you please also add some documentation ? You can make a separate doc file if you wish (markdown format is preferred)

@bchoquet-heliopsis

This comment has been minimized.

Show comment Hide comment
@bchoquet-heliopsis

bchoquet-heliopsis Apr 13, 2012

Contributor

Alright, I made the changes and redacted the doc. I'm a bit puzzled with markdown syntax though: HTML in code blocks do not appear when viewing file on Github...

Contributor

bchoquet-heliopsis commented Apr 13, 2012

Alright, I made the changes and redacted the doc. I'm a bit puzzled with markdown syntax though: HTML in code blocks do not appear when viewing file on Github...

+class SQLIImportInvalidFileFormatException extends SQLIImportBaseException
+{
+
+}

This comment has been minimized.

Show comment Hide comment
@lolautruche

lolautruche Nov 28, 2013

Owner

Missing empty line at end of file

@lolautruche

lolautruche Nov 28, 2013

Owner

Missing empty line at end of file

+ * @throws SQLIImportInvalidFileFormatException
+ */
+ public function validateFile( $option, $filePath );
+}

This comment has been minimized.

Show comment Hide comment
@lolautruche

lolautruche Nov 28, 2013

Owner

Missing empty line at end of file

@lolautruche

lolautruche Nov 28, 2013

Owner

Missing empty line at end of file

@@ -77,15 +77,20 @@ public static function fromText( $textOptions )
public function toText()
{
$text = '';
+ if( empty( $this->properties ) )
+ {
+ return '';

This comment has been minimized.

Show comment Hide comment
@lolautruche

lolautruche Nov 28, 2013

Owner

You should either return $text or define $text below the condition

@lolautruche

lolautruche Nov 28, 2013

Owner

You should either return $text or define $text below the condition

+class SQLIUsersImportHandler extends SQLIImportAbstractHandler implements ISQLIFileImportHandler
+{
+
+ protected $csv;

This comment has been minimized.

Show comment Hide comment
@lolautruche

lolautruche Nov 28, 2013

Owner

Missing PHPDoc. Please document at least which type this is.

@lolautruche

lolautruche Nov 28, 2013

Owner

Missing PHPDoc. Please document at least which type this is.

+<?php
+class SQLIUsersImportHandler extends SQLIImportAbstractHandler implements ISQLIFileImportHandler
+{
+

This comment has been minimized.

Show comment Hide comment
@lolautruche

lolautruche Nov 28, 2013

Owner

CS: Empty line

@lolautruche

lolautruche Nov 28, 2013

Owner

CS: Empty line

+ return true;
+ }
+
+}

This comment has been minimized.

Show comment Hide comment
@lolautruche

lolautruche Nov 28, 2013

Owner

Missing empty line at end of file

@lolautruche

lolautruche Nov 28, 2013

Owner

Missing empty line at end of file

+
+ $handler = $args[0];
+
+ //arg 2 may be a scheduled import id

This comment has been minimized.

Show comment Hide comment
@lolautruche

lolautruche Nov 28, 2013

Owner

CS: Comment not aligned

@lolautruche

lolautruche Nov 28, 2013

Owner

CS: Comment not aligned

+ {
+ throw new SQLIImportRuntimeException( 'No config for handler ' . $handler );
+ }
+

This comment has been minimized.

Show comment Hide comment
@lolautruche

lolautruche Nov 28, 2013

Owner

CS: Extra empty line

@lolautruche

lolautruche Nov 28, 2013

Owner

CS: Extra empty line

+ {
+ $value = $currentOptions->{$optionAlias};
+ }
+ elseif( isset( $optionsDefaults[ $optionAlias ] ) )

This comment has been minimized.

Show comment Hide comment
@lolautruche

lolautruche Nov 28, 2013

Owner

CS: No WS between [] (also for lines below)

@lolautruche

lolautruche Nov 28, 2013

Owner

CS: No WS between [] (also for lines below)

+
+ return $importHandler->validateFile( $option, $file->attribute( 'filename' ) );
+ }
+}

This comment has been minimized.

Show comment Hide comment
@lolautruche

lolautruche Nov 28, 2013

Owner

Missing empty line at end of file

@lolautruche

lolautruche Nov 28, 2013

Owner

Missing empty line at end of file

+
+}, '0.0.1', {
+ requires: [ 'sqliimport', 'uploader', 'json-parse' ]
+});

This comment has been minimized.

Show comment Hide comment
@lolautruche

lolautruche Nov 28, 2013

Owner

Missing empty line at end of file

@lolautruche

lolautruche Nov 28, 2013

Owner

Missing empty line at end of file

+ }
+ }
+ });
+});

This comment has been minimized.

Show comment Hide comment
@lolautruche

lolautruche Nov 28, 2013

Owner

Missing empty line at end of file

@lolautruche

lolautruche Nov 28, 2013

Owner

Missing empty line at end of file

+}, '0.0.1', {
+ //set your module dependencies here
+ requires: [ 'sqliimport' ]
+});

This comment has been minimized.

Show comment Hide comment
@lolautruche

lolautruche Nov 28, 2013

Owner

Missing empty line at end of file

@lolautruche

lolautruche Nov 28, 2013

Owner

Missing empty line at end of file

@@ -0,0 +1 @@
+<input type="checkbox" name="ImportOption_{$option_id}" value="1"{if $value|eq(1)} checked="checked"{/if} />

This comment has been minimized.

Show comment Hide comment
@lolautruche

lolautruche Nov 28, 2013

Owner

Missing empty line at end of file

@lolautruche

lolautruche Nov 28, 2013

Owner

Missing empty line at end of file

+ </span>
+ <p class="sqliimport-option-fileupload-filename"></p>
+</div>
+{set $jsModules = $jsModules|merge( array( 'uploader', 'sqliimportfileupload' ) )}

This comment has been minimized.

Show comment Hide comment
@lolautruche

lolautruche Nov 28, 2013

Owner

Missing empty line at end of file

@lolautruche

lolautruche Nov 28, 2013

Owner

Missing empty line at end of file

+## A Note on backwards compatibility
+New options implementation is fully compatible with previous behaviour (options string defined in a single textarea).
+If you wish to keep it old school, do not define any option and set the following value in `sqliimport.ini` :
+<pre><code>

This comment has been minimized.

Show comment Hide comment
@lolautruche

lolautruche Nov 28, 2013

Owner

Instead of <pre><code>, you can just use the triple back tick ```, followed by the format. See GitHub Flavored Markdown.

@lolautruche

lolautruche Nov 28, 2013

Owner

Instead of <pre><code>, you can just use the triple back tick ```, followed by the format. See GitHub Flavored Markdown.

+ <requires>
+ <extension name="ezjscore" />
+ </requires>
+ </dependencies>

This comment has been minimized.

Show comment Hide comment
@lolautruche

lolautruche Nov 28, 2013

Owner

Indentation not correct.

@lolautruche

lolautruche Nov 28, 2013

Owner

Indentation not correct.

@lolautruche

This comment has been minimized.

Show comment Hide comment
@lolautruche

lolautruche Nov 28, 2013

Owner

OK, I think we're good, apart the various notes I left.
Please note that for every file added, you miss an empty line at the end of it.

A rebase is also needed.

Good job ! 👍

Owner

lolautruche commented Nov 28, 2013

OK, I think we're good, apart the various notes I left.
Please note that for every file added, you miss an empty line at the end of it.

A rebase is also needed.

Good job ! 👍

@lolautruche

This comment has been minimized.

Show comment Hide comment
@lolautruche

lolautruche Feb 6, 2014

Owner

Ping @bchoquet-heliopsis
Can you please fix the left overs and rebase ?
Also, one or two screenshots in the PR description would be nice 😃

Thanks !

Owner

lolautruche commented Feb 6, 2014

Ping @bchoquet-heliopsis
Can you please fix the left overs and rebase ?
Also, one or two screenshots in the PR description would be nice 😃

Thanks !

@bchoquet-heliopsis

This comment has been minimized.

Show comment Hide comment
@bchoquet-heliopsis

bchoquet-heliopsis Feb 6, 2014

Contributor

Well I missed your comments from 2 months ago. Longest PR ever, I thought it was dead.
I'll try to find some time but it may not be soon.

Contributor

bchoquet-heliopsis commented Feb 6, 2014

Well I missed your comments from 2 months ago. Longest PR ever, I thought it was dead.
I'll try to find some time but it may not be soon.

@lolautruche

This comment has been minimized.

Show comment Hide comment
@lolautruche

lolautruche Jun 6, 2014

Owner
@bchoquet-heliopsis

This comment has been minimized.

Show comment Hide comment
@bchoquet-heliopsis

bchoquet-heliopsis Jun 6, 2014

Contributor

Sorry but I definitely don't have time to get back to this right now and it's not a priority at all...
But feel free to make the changes yourself :)

Contributor

bchoquet-heliopsis commented Jun 6, 2014

Sorry but I definitely don't have time to get back to this right now and it's not a priority at all...
But feel free to make the changes yourself :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment