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.

Sign up for GitHub

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

Use new inputs from UI-framework in Study Programme #1189

Merged
merged 7 commits into from
Oct 25, 2018
+109 −95
Merged

Use new inputs from UI-framework in Study Programme #1189

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8397351
SP: switched settings form to ui-framework
klees Sep 12, 2018
1917a54
SP: correction in form-handling
klees Sep 18, 2018
e7b7a64
SP: minor wording in comment
klees Sep 18, 2018
1dc3cae
SP: use text-area for description in settings
klees Oct 4, 2018
7b47574
SP: Status is a required setting
klees Oct 4, 2018
3c7b2c1
SP: consider 'no_type' in new form implementation
klees Oct 23, 2018
8122dcb
SP: fixed update after new form implementation
klees Oct 23, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
197 changes: 107 additions & 90 deletions Modules/StudyProgramme/classes/class.ilObjStudyProgrammeSettingsGUI.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,26 @@ class ilObjStudyProgrammeSettingsGUI {
*/
protected $tmp_heading;

/**
* @var ILIAS\UI\Component\Input\Factory
*/
protected $input_factory;

/**
* @var ILIAS\UI\Renderer
*/
protected $renderer;

/**
* @var Psr\Http\Message\ServerRequestInterface
*/
protected $request;

/**
* @var ILIAS\Transformation\Factory
*/
protected $trafo_factory;

public function __construct($a_parent_gui, $a_ref_id) {
global $DIC;
$tpl = $DIC['tpl'];
Expand All @@ -86,6 +106,10 @@ public function __construct($a_parent_gui, $a_ref_id) {
$this->ilLog = $ilLog;
$this->ilias = $ilias;
$this->lng = $lng;
$this->input_factory = $DIC->ui()->factory()->input();
$this->renderer = $DIC->ui()->renderer();
$this->request = $DIC->http()->request();
$this->trafo_factory = new \ILIAS\Transformation\Factory(); // TODO: replace this with the version from the DIC once available

$this->object = null;

Expand Down Expand Up @@ -128,27 +152,25 @@ public function executeCommand() {
protected function view() {
$this->buildModalHeading($this->lng->txt('prg_async_settings'),isset($_GET["currentNode"]));

$form = $this->buildForm();
$this->fillForm($form);
return $form->getHTML();
$form = $this->buildForm($this->getObject(), $this->ctrl->getFormAction($this, "update"));
return $this->renderer->render($form);
}

/*protected function cancel() {
$this->ctrl->redirect($this->parent_gui);
}*/

protected function update() {
$form = $this
->buildForm($this->getObject(), $this->ctrl->getFormAction($this, "update"))
->withRequest($this->request);
$content = $form->getData();
$prg = $this->getObject();

$form = $this->buildForm();
$form->setValuesByPost();
$update_possible = $this->checkForm($form);

// This could further improved by providing a new container for asynch-forms in the
// UI-Framework.
$update_possible = !is_null($content);
if ($update_possible) {
$this->updateFromFrom($form);
$this->updateWith($prg, $content);
ilUtil::sendSuccess($this->lng->txt("msg_obj_modified"),true);
$response = ilAsyncOutputHandler::encodeAsyncResponse(array("success"=>true, "message"=>$this->lng->txt("msg_obj_modified")));
} else {
// TODO:
ilUtil::sendFailure($this->lng->txt("msg_form_save_error"));
$response = ilAsyncOutputHandler::encodeAsyncResponse(array("success"=>false, "errors"=>$form->getErrors()));
}
Expand All @@ -159,7 +181,7 @@ protected function update() {
if($update_possible) {
$this->ctrl->redirect($this);
} else {
return $form->getHTML();
return $this->renderer->render($form);
}
}
}
Expand Down Expand Up @@ -191,50 +213,80 @@ protected function buildModalHeading($label, $current_node) {
const PROP_POINTS = "points";
const PROP_STATUS = "status";

protected function buildForm() {
$form = new ilAsyncPropertyFormGUI();
protected function buildForm(\ilObjStudyProgramme $prg, string $submit_action) : ILIAS\UI\Component\Input\Container\Form\Standard {
$ff = $this->input_factory->field();
$tf = $this->trafo_factory;
$txt = function($id) { return $this->lng->txt($id); };
$sp_types = ilStudyProgrammeType::getAllTypesArray();
$status_options = self::getStatusOptions();
return $this->input_factory->container()->form()->standard(
$submit_action,
[
$ff->section(
[
self::PROP_TITLE =>
$ff->text($txt("title"))
->withValue($prg->getTitle())
->withRequired(true),
self::PROP_DESC =>
$ff->textarea($txt("description"))
->withValue($prg->getDescription())
],
$txt("prg_edit"),
""
Copy link
Member Author

@klees klees Sep 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

One thing I do not like is, that the title and description go after the content when constructing a section.

),
$ff->section(
[
self::PROP_TYPE =>
$ff->select($txt("type"), $sp_types)
->withValue($prg->getSubtypeId() == 0 ? "" : $prg->getSubtypeId())
->withAdditionalTransformation($tf->custom(function($v) {
if ($v == "") {
return 0;
}
return $v;
}))
],
$txt("prg_type"),
""
),
$ff->section(
[
self::PROP_POINTS =>
$ff->numeric($txt("prg_points"))
->withValue((string)$prg->getPoints()),
self::PROP_STATUS =>
$ff->select($txt("prg_status"), $status_options)
->withValue((string)$prg->getStatus())
->withRequired(true)
],
$txt("prg_assessment"),
""
)
]
)
->withAdditionalTransformation($tf->custom(function($values) {
// values now contains the results of the single sections,
// i.e. a list of arrays that each contains keys according
// to the section they originated from.
return call_user_func_array("array_merge", $values);
}));
}
Copy link
Contributor

@Amstutz Amstutz Sep 19, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I do not really like that much nesting in terms of shuffling into arrays each other. However if you look at this, note, that you might also generate your inputs like so:

$input_a = $ff->numeric($txt("prg_points"))->...
$input_b = $ff->select($txt("prg_status"), ....

an then pack them subsequentially:
$ff->section([$input_a ,$input_b],$txt("prg_assessment"),"")

and so on. I think there is no good or bad way here, it's just a matter of personal taste.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I partially agree. In the style I used, without temporary variables, one directly finds the structure of the form as it is shown on the GUI without following references. In general I like that, but this might also get out of hand at some point. ATM I think I would start to use methods for the single sections once the form gets bigger then, say, one page one my screen. I would use methods (and not temporary variables) because this would allow me to have a reading flow from general-form-layout to specific sections and inputs, while I would need to define the specifics (sections, inputs, etc.) before the general structure if I use variables.


if(!$this->ctrl->isAsynch()) {
$form->setAsync(false);
protected function updateWith(\ilObjStudyProgramme $prg, array $data) {
$prg->setTitle($data[self::PROP_TITLE]);
$prg->setDescription($data[self::PROP_DESC]);

if($prg->getSubtypeId() != $data[self::PROP_TYPE]) {
$prg->setSubtypeId($data[self::PROP_TYPE]);
$prg->updateCustomIcon();
$this->parent_gui->setTitleAndDescription();
}

$form->setFormAction($this->ctrl->getFormAction($this));

$header = new ilFormSectionHeaderGUI();
$header->setTitle($this->lng->txt("prg_edit"));
$form->addItem($header);

$item = new ilTextInputGUI($this->lng->txt("title"), self::PROP_TITLE);
$item->setRequired(true);
$form->addItem($item);

$item = new ilTextAreaInputGUI($this->lng->txt("description"), self::PROP_DESC);
$form->addItem($item);

$header = new ilFormSectionHeaderGUI();
$header->setTitle($this->lng->txt("prg_type"));
$form->addItem($header);

$item = new ilSelectInputGUI($this->lng->txt("type"), self::PROP_TYPE);
$item->setOptions(ilStudyProgrammeType::getAllTypesArray());
$form->addItem($item);

$header = new ilFormSectionHeaderGUI();
$header->setTitle($this->lng->txt("prg_assessment"));
$form->addItem($header);

$item = new ilNumberInputGUI($this->lng->txt("prg_points"), self::PROP_POINTS);
$item->setMinValue(0);
$form->addItem($item);

$item = new ilSelectInputGUI($this->lng->txt("prg_status"), self::PROP_STATUS);
$item->setOptions(self::getStatusOptions());
$form->addItem($item);

$form->addCommandButton("update", $this->lng->txt("save"));
$form->addCommandButton("cancel", $this->lng->txt("cancel"));

return $form;
$prg->setPoints($data[self::PROP_POINTS]);
Copy link
Contributor

@mjansenDatabay mjansenDatabay Sep 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

What about input filtering (at least with known mechanisms like ilUtil::stripSlashes() implicitly done in legacy form handling)? Currently the raw HTTP post data (checked with constraints) is persisted in database by calling $prg->update();. I don't see any sanitizing step here. When merging this, this process will be (probably more) vulnerable against XSS (as with legacy forms).

Copy link
Member Author

Choose a reason for hiding this comment

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

Although $data might look superficially similar to raw HTTP-post data, it in fact is already processed and "filtered". The core of this functionality is here. The values supplied via post are checked with isClientSideValueOk first. This is is_numeric for the Numeric Field. If this does not work, an exception is thrown. The values are then checked and transformed via additional validations and transformations, where Numeric Field again checks for is_numeric via a constraint (which is already to much, because the value certainly is numeric already) Only if this worked for every field in the form, the user gets the checked and transformed data from Form::getData. There might be bugs in this mechanism (as always) and there are fields (I'm looking at you, Select Field) that are missing necessary checks, but this is in now way raw data from HTTP-Post. Also I cannot see why we would want to treat every input with ilUtil::stripSlashes if there are simpler means (like checking is_numeric for the Numeric Field, or checking against available options for Select Field) to discard malicious input. We might need to discuss how to treat general text-input in that regard, though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe my comment points to the wrong line of code. It is not about the numeric fields (here: points). It's about the title and description. I could not see any constraints bound to these fields. And of course something like ilUtil::stripSlashes() MUST NOT be part of the Input service, but I expected something like a MustNotContainInvalidChars constraint or some kind of transformation (making use of \ilUtil::stripSlashes(), or the Input filter service in future) for the text fields already shipped with this initial/first form migration.

$prg->setStatus($data[self::PROP_STATUS]);

$prg->update();
}

protected function getObject() {
Expand All @@ -244,41 +296,6 @@ protected function getObject() {
return $this->object;
}

protected function fillForm($a_form) {
$obj = $this->getObject();

$a_form->setValuesByArray(array
( self::PROP_TITLE => $obj->getTitle()
, self::PROP_DESC => $obj->getDescription()
, self::PROP_TYPE => $obj->getSubtypeId()
, self::PROP_POINTS => $obj->getPoints()
, self::PROP_STATUS => $obj->getStatus()
));
}

protected function checkForm($a_form) {
if (!$a_form->checkInput()) {
return false;
}
return true;
}

protected function updateFromFrom($a_form) {
$obj = $this->getObject();

$obj->setTitle($a_form->getItemByPostVar(self::PROP_TITLE)->getValue());
$obj->setDescription($a_form->getItemByPostVar(self::PROP_DESC)->getValue());

if($obj->getSubtypeId() != $a_form->getItemByPostVar(self::PROP_TYPE)->getValue()) {
$obj->setSubtypeId($a_form->getItemByPostVar(self::PROP_TYPE)->getValue());
$obj->updateCustomIcon();
$this->parent_gui->setTitleAndDescription();
}

$obj->setPoints($a_form->getItemByPostVar(self::PROP_POINTS)->getValue());
$obj->setStatus($a_form->getItemByPostVar(self::PROP_STATUS)->getValue());
}

static protected function getStatusOptions() {
global $DIC;
$lng = $DIC['lng'];
Expand Down
7 changes: 2 additions & 5 deletions Modules/StudyProgramme/classes/model/class.ilStudyProgrammeType.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,16 +167,13 @@ public static function getAllTypes() {
return self::get();
}

public static function getAllTypesArray($add_empty_on_begin = true) {
public static function getAllTypesArray() {
$out = array();

foreach(self::getAllTypes() as $type) {
$out[$type->getId()] = $type->getTitle();
}

if($add_empty_on_begin)
$out = array('-'=>'-') + $out;

return $out;
}

Expand Down Expand Up @@ -987,4 +984,4 @@ public function getCreateDate() {
return $this->create_date;
}

}
}