[FIX] ADF-3: child items of the class should show file property #2979
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
shpran
reviewed
Jun 25, 2021
helpers/form/class.Form.php
Outdated
Comment on lines
624
to
633
| $returnValue = false; | ||
|
|
||
| foreach ($this->elements as $element) { | ||
| if ($element instanceof tao_helpers_form_elements_xhtml_AsyncFile) { | ||
| $returnValue = true; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| return $returnValue; |
There was a problem hiding this comment.
Suggested change
| $returnValue = false; | |
| foreach ($this->elements as $element) { | |
| if ($element instanceof tao_helpers_form_elements_xhtml_AsyncFile) { | |
| $returnValue = true; | |
| break; | |
| } | |
| } | |
| return $returnValue; | |
| foreach ($this->elements as $element) { | |
| if ($element instanceof tao_helpers_form_elements_xhtml_AsyncFile) { | |
| return true; | |
| } | |
| } | |
| return false; |
helpers/form/ElementMapFactory.php
Outdated
Comment on lines
70
to
73
| // if ($widgetUri === AsyncFile::WIDGET_ID) { | ||
| // $widgetResource = new core_kernel_classes_Resource(GenerisAsyncFile::WIDGET_ID); | ||
| // } | ||
|
|
There was a problem hiding this comment.
If we don't need this part - just remove it
gabrielfs7
reviewed
Jun 25, 2021
gabrielfs7
reviewed
Jun 25, 2021
There was a problem hiding this comment.
- New code is covered by tests (if applicable)
- Tests are running successfully (old and new ones) on my local machine (if applicable)
- New code is respecting code style rules
- New code is respecting best practices
- New code is not subject to concurrency issues (if applicable)
- Feature is working correctly on my local machine (if applicable)
- Acceptance criteria are respected
- Pull request title and description are meaningful
Version
There are 0 BREAKING CHANGE, 0 feature, 2 fixes |
gabrielfs7
reviewed
Jun 30, 2021
gabrielfs7
requested changes
Jun 30, 2021
habemuscode
approved these changes
Jun 30, 2021
There was a problem hiding this comment.
This must be merged after merge PR of tao-core-ui and release it, then change here the related files (Ping me)
- New code is covered by tests (if applicable)
- Tests are running successfully (old and new ones) on my local machine (if applicable)
- New code is respecting code style rules
- New code is respecting best practices
- New code is not subject to concurrency issues (if applicable)
- Feature is working correctly on my local machine (if applicable)
- Acceptance criteria are respected
- Pull request title and description are meaningful
gabrielfs7
reviewed
Jul 1, 2021
| public function checkFormInstance(): bool | ||
| { | ||
| foreach ($this->elements as $element) { | ||
| if ($element->getName() == "tao.forms.instance" ) { |
There was a problem hiding this comment.
What do you think of having a constant added at tao_actions_form_Instance for tao.forms.instance?
There was a problem hiding this comment.
tao.forms.instance is the name of the object of class tao_helpers_form_elements_xhtml_Hidden
There was a problem hiding this comment.
I know, but it is repeating now, so I suggested to create a constant instead of repeat.
gabrielfs7
reviewed
Jul 1, 2021
helpers/form/class.Form.php
Outdated
| return false; | ||
| } | ||
|
|
||
| public function checkFormInstance(): bool |
There was a problem hiding this comment.
IMO check seems you are validating something, which is not the case. You just need to know if this is a form instance.
Suggested change
| public function checkFormInstance(): bool | |
| public function isFormInstance(): bool |
gabrielfs7
reviewed
Jul 1, 2021
…com/oat-sa/tao-core into feature/ADF-3/missing-file-property
gabrielfs7
reviewed
Jul 2, 2021
| public function setUp(): void | ||
| { | ||
| $this->subject = new tao_helpers_form_xhtml_Form(); | ||
| $this->tao_helpers_form_xhtml_Form_obj = $this->createMock(tao_helpers_form_xhtml_Form::class); |
There was a problem hiding this comment.
Do not you think this name is too big? Also we do not use _ or complete namespace in the variables.
Suggested change
| $this->tao_helpers_form_xhtml_Form_obj = $this->createMock(tao_helpers_form_xhtml_Form::class); | |
| $this->form = $this->createMock(tao_helpers_form_xhtml_Form::class); |
gabrielfs7
reviewed
Jul 2, 2021
Comment on lines
41
to
42
| $response = $this->subject->hasAsyncFileUpload($this->tao_helpers_form_xhtml_Form_obj); | ||
| $this->assertIsBool($response); |
There was a problem hiding this comment.
Unnecessary variable.
Suggested change
| $response = $this->subject->hasAsyncFileUpload($this->tao_helpers_form_xhtml_Form_obj); | |
| $this->assertIsBool($response); | |
| $this->assertIsBool($this->subject->hasAsyncFileUpload($this->tao_helpers_form_xhtml_Form_obj)); |
gabrielfs7
reviewed
Jul 2, 2021
| $this->tao_helpers_form_xhtml_Form_obj = $this->createMock(tao_helpers_form_xhtml_Form::class); | ||
| } | ||
|
|
||
| public function testHasAsyncFileUpload(): void |
gabrielfs7
reviewed
Jul 2, 2021
| $this->assertIsBool($response); | ||
| } | ||
|
|
||
| public function testIsFormInstance(): void |
gabrielfs7
reviewed
Jul 2, 2021
Comment on lines
47
to
48
| $response = $this->subject->isFormInstance($this->tao_helpers_form_xhtml_Form_obj); | ||
| $this->assertIsBool($response); |
There was a problem hiding this comment.
Suggested change
| $response = $this->subject->isFormInstance($this->tao_helpers_form_xhtml_Form_obj); | |
| $this->assertIsBool($response); | |
| $this->assertIsBool($this->subject->isFormInstance($this->tao_helpers_form_xhtml_Form_obj)); |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related task - https://oat-sa.atlassian.net/browse/ADF-3
Description: User has the possibility to add additional property for a given folder/class, using the "Manage Schema" option. All the items that are contained by this class will inherit the parent-folder's properties.
It works for all the properties BUT the File one.
Upon adding of this property, even though set for the parent-folder, the items won't inherit this only property.
Expected Result: all the child elements of the class are inheriting and are being shown all its properties.