[#406] Added datetime handling. #409
Conversation
|
""" WalkthroughSupport for datetime fields has been added to the Changes
Sequence Diagram(s)sequenceDiagram
participant Tester
participant FieldTrait
participant Form
Tester->>FieldTrait: fieldFillDateTime(label, date, time)
FieldTrait->>Form: Locate date input by label
FieldTrait->>Form: Set date input value
FieldTrait->>Form: Locate time input by label
FieldTrait->>Form: Set time input value
Form-->>Tester: Datetime field filled
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (23)
🔇 Additional comments (41)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
STEPS.md(2 hunks)src/FieldTrait.php(2 hunks)tests/behat/features/field_datetime.feature(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
STEPS.md
484-484: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
484-484: Multiple spaces after blockquote symbol
null
(MD027, no-multiple-space-blockquote)
515-515: Inline HTML
Element: details
(MD033, no-inline-html)
516-516: Inline HTML
Element: summary
(MD033, no-inline-html)
516-516: Inline HTML
Element: code
(MD033, no-inline-html)
518-518: Inline HTML
Element: br
(MD033, no-inline-html)
520-520: Inline HTML
Element: br
(MD033, no-inline-html)
520-520: Inline HTML
Element: br
(MD033, no-inline-html)
529-529: Inline HTML
Element: details
(MD033, no-inline-html)
530-530: Inline HTML
Element: summary
(MD033, no-inline-html)
530-530: Inline HTML
Element: code
(MD033, no-inline-html)
532-532: Inline HTML
Element: br
(MD033, no-inline-html)
534-534: Inline HTML
Element: br
(MD033, no-inline-html)
534-534: Inline HTML
Element: br
(MD033, no-inline-html)
543-543: Inline HTML
Element: details
(MD033, no-inline-html)
544-544: Inline HTML
Element: summary
(MD033, no-inline-html)
544-544: Inline HTML
Element: code
(MD033, no-inline-html)
546-546: Inline HTML
Element: br
(MD033, no-inline-html)
548-548: Inline HTML
Element: br
(MD033, no-inline-html)
548-548: Inline HTML
Element: br
(MD033, no-inline-html)
🔇 Additional comments (7)
src/FieldTrait.php (2)
18-18: Documentation update correctly reflects new functionality.The trait documentation has been appropriately updated to mention the new support for datetime field widgets.
211-223: Implementation for datetime field handling looks good.The new public method provides a clean interface for filling datetime fields with separate date and time components. The method signature and documentation are clear and follow the established pattern in the trait.
STEPS.md (2)
484-484: Documentation update properly reflects the new functionality.The FieldTrait description has been updated to include support for datetime field widgets.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
484-484: Unordered list indentation
Expected: 0; Actual: 1(MD007, ul-indent)
484-484: Multiple spaces after blockquote symbol
null(MD027, no-multiple-space-blockquote)
543-555: Step definition for datetime fields matches implementation.This documentation correctly describes the implemented datetime field functionality and provides a clear example that matches the implementation in FieldTrait.php.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
543-543: Inline HTML
Element: details(MD033, no-inline-html)
544-544: Inline HTML
Element: summary(MD033, no-inline-html)
544-544: Inline HTML
Element: code(MD033, no-inline-html)
546-546: Inline HTML
Element: br(MD033, no-inline-html)
548-548: Inline HTML
Element: br(MD033, no-inline-html)
548-548: Inline HTML
Element: br(MD033, no-inline-html)
tests/behat/features/field_datetime.feature (3)
1-17: Test scenario validates the datetime field functionality.This is a well-structured Behat test scenario that:
- Sets up the context (admin user, content creation form)
- Exercises the new datetime field functionality
- Verifies the expected output (field label and formatted datetime value)
The test provides good coverage for the newly added feature.
11-11: Usage example aligns with implementation.This line demonstrates the proper usage of the new step definition for filling in a datetime field, matching the implementation in FieldTrait.php and the documentation in STEPS.md.
17-17: Verification of formatted output is a good practice.Checking for the formatted datetime output validates that the field was not only filled but also properly processed and displayed by the application.
| throw new \Exception(sprintf('Date field with label "%s" value not found', $label)); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Error message could be more specific.
The error message only mentions "Date field" even when the method might be looking for a time input element.
Consider making the error message reflect the actual field type being searched:
- throw new \Exception(sprintf('Date field with label "%s" value not found', $label));
+ throw new \Exception(sprintf('%s field with label "%s" value not found', ucfirst($field), $label));Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/FieldTrait.php around lines 251 to 252, the error message always refers
to a "Date field" even when the method might be searching for a time input
element. Modify the error message to dynamically include the actual field type
being searched for, such as "Date field" or "Time field," to make the error more
specific and informative.
STEPS.md
Outdated
| <details> | ||
| <summary><code>@When I fill in the time field :label with :time</code></summary> | ||
|
|
||
| <br/> | ||
| Fill a time field with a time value | ||
| <br/><br/> | ||
|
|
||
| ```gherkin | ||
| When I fill in the time field "Event time" with "14:30:00" | ||
| ``` | ||
|
|
||
| </details> |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Step definition for time fields is well documented.
The documentation for filling time fields is clear and includes a proper Gherkin syntax example.
Similarly, please verify that this step definition is implemented elsewhere or needs to be added:
🏁 Script executed:
#!/bin/bash
# Check if there's an implementation for the time field step
grep -r "fill in the time field" src/Length of output: 39
🏁 Script executed:
#!/bin/bash
# Search entire repository for any implementation of the "fill in the time field" step
grep -R "fill in the time field" -n .
grep -R "I fill in the time field" -n .
# Also look specifically in the typical Cucumber step definitions folder
grep -R "time field" -n features/step_definitionsLength of output: 536
Missing step definition for “fill in the time field”
The documentation in STEPS.md defines the Gherkin step, but no matching implementation exists under features/step_definitions. Please add or update a step definition. For example:
# File: features/step_definitions/time_field_steps.rb
When(/^I fill in the time field "([^"]+)" with "([^"]+)"$/) do |label, time|
fill_in(label, with: time)
end- Create features/step_definitions/time_field_steps.rb (if it doesn’t exist)
- Implement the step as shown (or adjust to your project’s conventions)
- Ensure this file is loaded by Cucumber
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
529-529: Inline HTML
Element: details
(MD033, no-inline-html)
530-530: Inline HTML
Element: summary
(MD033, no-inline-html)
530-530: Inline HTML
Element: code
(MD033, no-inline-html)
532-532: Inline HTML
Element: br
(MD033, no-inline-html)
534-534: Inline HTML
Element: br
(MD033, no-inline-html)
534-534: Inline HTML
Element: br
(MD033, no-inline-html)
🤖 Prompt for AI Agents
In STEPS.md around lines 529 to 541, the Gherkin step for filling in a time
field is documented but lacks a corresponding step definition implementation in
features/step_definitions. To fix this, create a new Ruby step definition file
named time_field_steps.rb under features/step_definitions if it does not exist,
and add a step definition matching the documented step that uses fill_in to set
the time field value. Ensure this step definition file is properly loaded by
Cucumber during test runs.
STEPS.md
Outdated
| <details> | ||
| <summary><code>@When I fill in the date field :label with :date</code></summary> | ||
|
|
||
| <br/> | ||
| Fill a date field with a date value | ||
| <br/><br/> | ||
|
|
||
| ```gherkin | ||
| When I fill in the date field "Event date" with "2024-07-15" | ||
| ``` | ||
|
|
||
| </details> |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Step definition for date fields is well documented.
The documentation for filling date fields is clear and includes a proper Gherkin syntax example.
It seems this step definition is documented but I don't see a corresponding implementation method in the FieldTrait.php file. Please verify that this step definition is implemented elsewhere or needs to be added:
🏁 Script executed:
#!/bin/bash
# Check if there's an implementation for the date field step
grep -r "fill in the date field" src/Length of output: 39
To ensure we didn’t miss a differently named implementation, let’s search the codebase for any mention of “date” in the context of FieldTrait and step annotations:
🏁 Script executed:
#!/bin/bash
# 1. Locate the FieldTrait.php file
field_trait=$(find src -type f -name FieldTrait.php)
echo "FieldTrait.php found at: $field_trait"
# 2. Check for any use of “date” within that trait
grep -R "date" -n "$field_trait"
# 3. Look for @When annotations mentioning “date field”
grep -R "@When.*date field" -n src/
# 4. Search for any method named fillInDateField
grep -R "fillInDateField" -n src/Length of output: 870
Missing step implementation for date fields
The documentation in STEPS.md clearly defines a
@When I fill in the date field :label with :date
step, but there’s no corresponding method in FieldTrait.php (or any Context) handling it. Please add something like:
/**
* Fill a date field with a date value.
*
* @When I fill in the date field :label with :date
*/
public function fieldFillDateField(string $label, string $date): void
{
// Reuse the datetime helper for the date component
$this->fieldFillDateTimeField($label, 'date', $date);
}• File: src/FieldTrait.php
• Location: alongside the existing fieldFillDateTime method
If date-only fields are meant to be covered by another implementation, please either add an alias there or update the documentation to avoid confusion.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
515-515: Inline HTML
Element: details
(MD033, no-inline-html)
516-516: Inline HTML
Element: summary
(MD033, no-inline-html)
516-516: Inline HTML
Element: code
(MD033, no-inline-html)
518-518: Inline HTML
Element: br
(MD033, no-inline-html)
520-520: Inline HTML
Element: br
(MD033, no-inline-html)
520-520: Inline HTML
Element: br
(MD033, no-inline-html)
🤖 Prompt for AI Agents
In STEPS.md lines 515 to 527, the step definition for filling a date field is
documented but lacks a corresponding implementation in src/FieldTrait.php or any
Context file. To fix this, add a new method in FieldTrait.php similar to the
existing datetime method, annotated with @When for "I fill in the date field
:label with :date". This method should accept the label and date string
parameters and internally call the datetime helper method with the 'date'
component to handle the input. This ensures the documented step is properly
implemented and functional.
skipper-vp
force-pushed
the
feature/406-datetime-handling
branch
from
c69397f to
23c6d1c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
STEPS.md (2)
515-527: 🛠️ Refactor suggestionMissing implementation for date field step definition
The documentation defines a step for filling in a date field, but the implementation in
src/FieldTrait.phponly includes the combined datetime method (fieldFillDateTime), not a dedicated method for date fields only.You should implement a dedicated method in
FieldTrait.phpto support this step:/** * Fill a date field with a date value. * * @code * When I fill in the date field "Event date" with "2024-07-15" * @endcode * * @When I fill in the date field :label with :date */ public function fieldFillDateField(string $label, string $date): void { $this->fieldFillDateTimeField($label, 'date', $date); }
529-541: 🛠️ Refactor suggestionMissing implementation for time field step definition
The documentation defines a step for filling in a time field, but the implementation in
src/FieldTrait.phponly includes the combined datetime method (fieldFillDateTime), not a dedicated method for time fields only.You should implement a dedicated method in
FieldTrait.phpto support this step:/** * Fill a time field with a time value. * * @code * When I fill in the time field "Event time" with "14:30:00" * @endcode * * @When I fill in the time field :label with :time */ public function fieldFillTimeField(string $label, string $time): void { $this->fieldFillDateTimeField($label, 'time', $time); }src/FieldTrait.php (2)
238-253: 🛠️ Refactor suggestionXPath selector should be more robust
The XPath selector assumes a specific HTML structure with the label in a span element with a specific parent-child relationship to the input fields.
Consider enhancing the method to be more robust by:
protected function fieldFillDateTimeField(string $label, string $field, string $value): void { - $xpath = sprintf( - '//span[contains(text(), "%s")]/../../div//input[contains(@name, "[%s]")]', - $label, - $field - ); + // Try multiple selector strategies to find the field + $xpaths = [ + // Strategy 1: Find by label text in a span element + sprintf('//span[contains(text(), "%s")]/../../div//input[contains(@name, "[%s]")]', $label, $field), + // Strategy 2: Find by label with 'for' attribute + sprintf('//label[contains(text(), "%s")]/@for', $label) . + sprintf('/following::input[contains(@name, "[%s]")]', $field), + // Strategy 3: Find by fieldset legend + sprintf('//fieldset//legend[contains(text(), "%s")]/..//input[contains(@name, "[%s]")]', $label, $field), + ]; + + $page = $this->getSession()->getPage(); + $element = null; + + // Try each strategy until we find a matching element + foreach ($xpaths as $xpath) { + $element = $page->find('xpath', $xpath); + if ($element instanceof NodeElement) { + break; + } + } - $page = $this->getSession()->getPage(); - $element = $page->find('xpath', $xpath); if ($element instanceof NodeElement) { $element->setValue($value); } else { - throw new \Exception(sprintf('Date field with label "%s" value not found', $label)); + throw new \Exception(sprintf('%s field with label "%s" value not found', ucfirst($field), $label)); } }
251-252: 🧹 Nitpick (assertive)Error message should reflect the field type
The error message always mentions "Date field" even when the method might be searching for a time input element.
- throw new \Exception(sprintf('Date field with label "%s" value not found', $label)); + throw new \Exception(sprintf('%s field with label "%s" value not found', ucfirst($field), $label));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Lite
📒 Files selected for processing (11)
STEPS.md(2 hunks)src/FieldTrait.php(2 hunks)tests/behat/features/field_datetime.feature(1 hunks)tests/behat/fixtures/d10/config/sync/core.entity_form_display.node.article.default.yml(3 hunks)tests/behat/fixtures/d10/config/sync/core.entity_view_display.node.article.default.yml(2 hunks)tests/behat/fixtures/d10/config/sync/core.entity_view_display.node.article.rss.yml(2 hunks)tests/behat/fixtures/d10/config/sync/core.entity_view_display.node.article.search_index.yml(2 hunks)tests/behat/fixtures/d10/config/sync/core.entity_view_display.node.article.search_result.yml(2 hunks)tests/behat/fixtures/d10/config/sync/core.entity_view_display.node.article.teaser.yml(2 hunks)tests/behat/fixtures/d10/config/sync/field.field.node.article.field_date_and_time.yml(1 hunks)tests/behat/fixtures/d10/config/sync/field.storage.node.field_date_and_time.yml(1 hunks)
🔇 Additional comments (18)
tests/behat/fixtures/d10/config/sync/core.entity_view_display.node.article.search_result.yml (2)
9-9: Addedfield_date_and_timeto view dependenciesThe new datetime field is correctly declared as a dependency in the
search_resultview display configuration.
57-57: Hide thefield_date_and_timein search resultsMarking the datetime field as hidden in this view mode aligns with the intended display settings.
tests/behat/fixtures/d10/config/sync/core.entity_view_display.node.article.search_index.yml (2)
10-10: Includefield_date_and_timein search index dependenciesThe dependency list now correctly includes the new datetime field for the
search_indexview mode.
55-55: Hide thefield_date_and_timein search indexThe datetime field is properly hidden in the search index view, matching the configuration pattern of other fields.
tests/behat/fixtures/d10/config/sync/core.entity_view_display.node.article.teaser.yml (2)
9-10: Dependencies updated for teaser viewBoth
field_date_and_timeandfield_fileare added to the teaser view’s dependency list, ensuring the new field is known to the config.
63-63: Hide thefield_date_and_timein teaser viewThe datetime field is correctly hidden in the teaser display, consistent with other view modes.
tests/behat/fixtures/d10/config/sync/core.entity_view_display.node.article.rss.yml (2)
9-10: Dependencies updated for RSS viewThe RSS view display now depends on
field_date_and_timealongside the existing fields.
34-34: Hide thefield_date_and_timein RSS outputThe datetime field is appropriately hidden in the RSS feed configuration.
tests/behat/fixtures/d10/config/sync/core.entity_form_display.node.article.default.yml (3)
8-8: Verify default view display config updatedI don’t see the corresponding default view display config (
core.entity_view_display.node.article.default.yml) in the fixtures; please confirm it’s been updated to include or hide thefield_date_and_timeconsistently.
18-18: Addeddatetimemodule dependency for form displayIncluding the
datetimemodule ensures the form widget for the new field will work as expected.
52-57: Configured new datetime field widgetThe
field_date_and_timeis correctly added to the form display with thedatetime_defaultwidget, weight, region, and default settings.STEPS.md (1)
543-555: Great documentation for the datetime field stepThe documentation for the datetime field step is clear and comprehensive, providing good examples for users.
tests/behat/features/field_datetime.feature (1)
1-17: Well-structured test for datetime field handlingThis feature test effectively validates the datetime field functionality, providing good coverage of the fill and display operations. The scenario follows a clear workflow from content creation to verification of the displayed values.
The test could be further enhanced in the future by adding scenarios for:
- Date-only field interactions
- Time-only field interactions
- Validation of incorrect date/time formats
tests/behat/fixtures/d10/config/sync/field.field.node.article.field_date_and_time.yml (1)
1-21: Correct field configuration for datetime implementationThe field configuration is correctly defined with appropriate dependencies and settings for a datetime field. The field label is clear and descriptive.
tests/behat/fixtures/d10/config/sync/field.storage.node.field_date_and_time.yml (1)
1-20: Correct field storage configurationThe field storage configuration is properly defined with the datetime type set to handle both date and time values. The settings are appropriate for a standard datetime field implementation.
src/FieldTrait.php (2)
18-18: Good documentation update for the traitThe trait documentation is clearly updated to include datetime field support alongside other field types.
211-224: Well-implemented datetime field handling methodThe
fieldFillDateTimemethod provides a clean interface for filling both date and time components of a datetime field, with good code documentation and examples.tests/behat/fixtures/d10/config/sync/core.entity_view_display.node.article.default.yml (1)
17-17: Confirm the module dependency.Including
datetimeunder module dependencies is correct to ensure the datetime UI formatter is available at import time. No further changes needed here.
| - core.entity_view_display.comment.comment.default | ||
| - field.field.node.article.body | ||
| - field.field.node.article.comment | ||
| - field.field.node.article.field_date_and_time |
There was a problem hiding this comment.
Validate configuration dependencies for field storage.
The new view display adds a dependency on the field config field.field.node.article.field_date_and_time but omits the required field storage config field.storage.node.field_date_and_time. Without this, importing the configuration may fail.
Apply this diff to add the storage dependency:
dependencies:
config:
- core.entity_view_display.comment.comment.default
- field.field.node.article.body
- field.field.node.article.comment
+ - field.storage.node.field_date_and_time
- field.field.node.article.field_date_and_time📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - field.field.node.article.field_date_and_time | |
| dependencies: | |
| config: | |
| - core.entity_view_display.comment.comment.default | |
| - field.field.node.article.body | |
| - field.field.node.article.comment | |
| - field.storage.node.field_date_and_time | |
| - field.field.node.article.field_date_and_time |
🤖 Prompt for AI Agents
In
tests/behat/fixtures/d10/config/sync/core.entity_view_display.node.article.default.yml
at line 9, the configuration dependencies are missing the required field storage
config 'field.storage.node.field_date_and_time'. Add this field storage
dependency alongside the existing field config dependency to ensure proper
configuration import and avoid failures.
| field_date_and_time: | ||
| type: datetime_default | ||
| label: above | ||
| settings: | ||
| timezone_override: '' | ||
| format_type: medium | ||
| third_party_settings: { } | ||
| weight: 7 | ||
| region: content |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Review view display settings for the datetime field.
The field_date_and_time display entry is correctly configured, but you may consider:
- Omitting the empty
timezone_override: ''if you don’t intend to override the default site timezone. - Removing the empty
third_party_settings: {}for brevity. - Verifying that
weight: 7places the field in the intended order relative to other content.
@@ content:
- field_date_and_time:
- type: datetime_default
- label: above
- settings:
- timezone_override: ''
- format_type: medium
- third_party_settings: { }
- weight: 7
- region: content
+ field_date_and_time:
+ type: datetime_default
+ label: above
+ settings:
+ format_type: medium
+ weight: 7 # ensure this ordering matches UX requirements
+ region: content📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| field_date_and_time: | |
| type: datetime_default | |
| label: above | |
| settings: | |
| timezone_override: '' | |
| format_type: medium | |
| third_party_settings: { } | |
| weight: 7 | |
| region: content | |
| field_date_and_time: | |
| type: datetime_default | |
| label: above | |
| settings: | |
| format_type: medium | |
| weight: 7 # ensure this ordering matches UX requirements | |
| region: content |
🤖 Prompt for AI Agents
In
tests/behat/fixtures/d10/config/sync/core.entity_view_display.node.article.default.yml
around lines 50 to 58, the datetime field display settings include an empty
timezone_override and an empty third_party_settings object, which are
unnecessary. Remove the empty timezone_override line and the empty
third_party_settings entry to clean up the configuration. Also, double-check
that the weight value of 7 correctly positions the field in the desired display
order relative to other fields.
23c6d1c to
48f6c68
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (4)
STEPS.md (2)
515-527: Verify existence of date field step implementation.
Ensure that the documented Gherkin step (@When I fill in the date field :label with :date) has a corresponding method annotated inFieldTrait.phpor another context file.
529-541: Verify existence of time field step implementation.
Confirm that there is a matching step definition for (@When I fill in the time field :label with :time) in the codebase to avoid broken tests.tests/behat/fixtures/d10/config/sync/core.entity_view_display.node.article.default.yml (1)
51-59: 🧹 Nitpick (assertive)Cleanup unnecessary empty settings.
Thetimezone_override: ''and emptythird_party_settings: {}entries can be removed for brevity:field_date_and_time: - settings: - timezone_override: '' - format_type: medium - third_party_settings: { } + settings: + format_type: mediumsrc/FieldTrait.php (1)
261-262: 🧹 Nitpick (assertive)Error message could be more specific.
The error message only mentions "Date field" even when the method might be looking for a time input element.
- throw new \Exception(sprintf('Date field with label "%s" value not found', $label)); + throw new \Exception(sprintf('%s field with label "%s" value not found', ucfirst($field), $label));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Lite
📒 Files selected for processing (13)
STEPS.md(2 hunks)src/FieldTrait.php(2 hunks)tests/behat/features/field_datetime.feature(1 hunks)tests/behat/fixtures/d10/config/sync/core.entity_form_display.node.article.default.yml(3 hunks)tests/behat/fixtures/d10/config/sync/core.entity_view_display.node.article.default.yml(2 hunks)tests/behat/fixtures/d10/config/sync/core.entity_view_display.node.article.rss.yml(2 hunks)tests/behat/fixtures/d10/config/sync/core.entity_view_display.node.article.search_index.yml(2 hunks)tests/behat/fixtures/d10/config/sync/core.entity_view_display.node.article.search_result.yml(2 hunks)tests/behat/fixtures/d10/config/sync/core.entity_view_display.node.article.teaser.yml(2 hunks)tests/behat/fixtures/d10/config/sync/field.field.node.article.field_date_and_time.yml(1 hunks)tests/behat/fixtures/d10/config/sync/field.field.node.article.field_date_only.yml(1 hunks)tests/behat/fixtures/d10/config/sync/field.storage.node.field_date_and_time.yml(1 hunks)tests/behat/fixtures/d10/config/sync/field.storage.node.field_date_only.yml(1 hunks)
🔇 Additional comments (23)
tests/behat/fixtures/d10/config/sync/core.entity_view_display.node.article.search_result.yml (2)
9-10: Add new datetime field dependencies
Addedfield.field.node.article.field_date_and_timeandfield.field.node.article.field_date_onlyto the dependencies list for thesearch_resultview mode.
58-59: Hide datetime fields in search_result view
Bothfield_date_and_timeandfield_date_onlyare correctly marked as hidden in this view mode to prevent them from displaying in search results.tests/behat/fixtures/d10/config/sync/core.entity_view_display.node.article.search_index.yml (2)
10-11: Add datetime field dependencies to search_index view
Thesearch_indexview mode dependencies now includefield.field.node.article.field_date_and_timeandfield.field.node.article.field_date_only, keeping consistency with other view modes.
56-57: Hide datetime fields in search_index view
The new datetime fields are correctly hidden under thehiddensection for thesearch_indexview mode.tests/behat/fixtures/d10/config/sync/core.entity_view_display.node.article.rss.yml (2)
9-11: Add datetime and file field dependencies to RSS view
Addedfield_date_and_time,field_date_only, andfield_fileto the dependencies list for the RSS view mode, ensuring the configuration is aware of these fields.
35-36: Hide datetime fields in RSS view
The new datetime fields are correctly set totrueunder the hidden section for the RSS view mode.tests/behat/fixtures/d10/config/sync/core.entity_view_display.node.article.teaser.yml (2)
9-11: Add datetime and file field dependencies to teaser view
The teaser view mode now depends onfield_date_and_time,field_date_only, andfield_file, aligning it with other view modes.
64-65: Hide datetime fields in teaser view
field_date_and_timeandfield_date_onlyare correctly hidden in the teaser view mode.tests/behat/fixtures/d10/config/sync/core.entity_form_display.node.article.default.yml (4)
8-9: Add datetime fields to form display dependencies
The form display config now includes dependencies onfield.field.node.article.field_date_and_timeandfield.field.node.article.field_date_only.
19-19: Add datetime module dependency
Including thedatetimemodule is required to support the new datetime widgets in the form display.
53-58: Configurefield_date_and_timewidget
Thefield_date_and_timefield is correctly configured with thedatetime_defaultwidget, weight, region, and empty settings.
59-64: Configurefield_date_onlywidget
Thefield_date_onlyfield is correctly configured with thedatetime_defaultwidget, weight, region, and empty settings.tests/behat/fixtures/d10/config/sync/field.field.node.article.field_date_only.yml (1)
1-21: Configuration forfield_date_onlyis correct.
Dependencies, settings (datetime_type: date), and module requirements are properly declared; no issues found.STEPS.md (2)
484-484: Approve update to FieldTrait documentation.
The new bullet point clarifying support for datetime field widgets is concise and aligns with the recent code additions.
543-556: Verify existence of datetime field step implementation.
Check that the step definition (@When I fill in the datetime field :label with date :date and time :time) is implemented and correctly wired to your helper methods.tests/behat/fixtures/d10/config/sync/field.storage.node.field_date_only.yml (1)
1-21: Valid field storage configuration.
The storage definition forfield_date_onlycorrectly specifiesdatetime_type: date, module dependencies, cardinality, and translatability. No issues found.tests/behat/fixtures/d10/config/sync/field.field.node.article.field_date_and_time.yml (1)
1-22: Field configuration forfield_date_and_timeis correct.
Dependencies, field type, and bundle configuration match expected requirements for date+time storage. Good to go.tests/behat/fixtures/d10/config/sync/core.entity_view_display.node.article.default.yml (1)
16-18: Module dependency inclusion is appropriate.
The addition ofdatetimeunder module dependencies ensures the formatter is available. No concerns.tests/behat/fixtures/d10/config/sync/field.storage.node.field_date_and_time.yml (1)
1-21: Configuration looks good and follows Drupal standards.The field storage configuration for the datetime field is properly set up with all required properties. The
datetime_type: datetimeconfiguration correctly enables both date and time storage, which aligns with the PR's objective of adding datetime handling.tests/behat/features/field_datetime.feature (1)
1-31: Feature file is well-structured and tests the core functionality.The feature file properly tests both datetime and date-only field types with appropriate assertions. The test cases follow a logical flow from field input to verification of displayed values and include proper setup with authentication.
src/FieldTrait.php (3)
18-18: Documentation update is clear and comprehensive.The class documentation has been appropriately updated to include the new datetime field handling capability.
211-237: New public methods for datetime fields are well-designed.The implementation of
fieldFillDateTimeandfieldFillDatefollows the established pattern in the trait. The methods have clear documentation with examples and appropriate method signatures.
252-257: XPath selector is more robust than previous implementations.The XPath selector has been significantly improved compared to previous implementations mentioned in past review comments. It now properly handles multiple HTML elements (span, label, h4) and different CSS classes, making it more flexible and robust.
| field_date_only: | ||
| type: datetime_default | ||
| label: above | ||
| settings: | ||
| timezone_override: '' | ||
| format_type: medium | ||
| third_party_settings: { } | ||
| weight: 8 | ||
| region: content |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Cleanup unnecessary empty settings.
Similarly, for field_date_only, remove empty timezone_override and third_party_settings:
field_date_only:
- settings:
- timezone_override: ''
- format_type: medium
- third_party_settings: { }
+ settings:
+ format_type: medium📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| field_date_only: | |
| type: datetime_default | |
| label: above | |
| settings: | |
| timezone_override: '' | |
| format_type: medium | |
| third_party_settings: { } | |
| weight: 8 | |
| region: content | |
| field_date_only: | |
| type: datetime_default | |
| label: above | |
| settings: | |
| format_type: medium | |
| weight: 8 | |
| region: content |
🤖 Prompt for AI Agents
In
tests/behat/fixtures/d10/config/sync/core.entity_view_display.node.article.default.yml
around lines 60 to 68, remove the empty settings 'timezone_override' under
'settings' and the empty 'third_party_settings' key from the 'field_date_only'
configuration to clean up unnecessary empty values.
| - field.field.node.article.field_date_and_time | ||
| - field.field.node.article.field_date_only |
There was a problem hiding this comment.
Add missing storage dependencies for datetime fields.
Configuration import may fail without including the corresponding storage dependencies. Please add:
dependencies:
config:
- field.field.node.article.field_date_and_time
+ - field.storage.node.field_date_and_time
- field.field.node.article.field_date_only
+ - field.storage.node.field_date_only🤖 Prompt for AI Agents
In
tests/behat/fixtures/d10/config/sync/core.entity_view_display.node.article.default.yml
around lines 9 to 10, the configuration is missing storage dependencies for the
datetime fields field_date_and_time and field_date_only. Add these storage
dependencies explicitly to ensure the configuration import succeeds without
errors related to missing dependencies.
| /** | ||
| * Helper method to fill datetime field components. | ||
| * | ||
| * @param string $label | ||
| * The field label. | ||
| * @param string $field | ||
| * The field component: 'date' or 'time'. | ||
| * @param string $value | ||
| * The value to set. | ||
| * | ||
| * @throws \Exception | ||
| * If the field is not found. | ||
| */ | ||
| protected function fieldFillDateTimeField(string $label, string $field, string $value): void { | ||
| $xpath = sprintf( | ||
| '//*[self::span or self::label or self::h4][contains(normalize-space(.), "%s")]/ancestor::div[contains(@class,"form-item") or contains(@class,"field--widget-datetime-default")]//input[contains(@name,"[%s]")]', | ||
| $label, | ||
| $field | ||
| ); | ||
|
|
||
| $page = $this->getSession()->getPage(); | ||
| $element = $page->find('xpath', $xpath); | ||
| if (!$element) { | ||
| throw new \Exception(sprintf('Date field with label "%s" value not found', $label)); | ||
| } | ||
| $element->setValue($value); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Helper method implementation is solid but could use additional validation.
The fieldFillDateTimeField helper method effectively handles the task of locating and filling datetime input fields. Consider adding validation for the $field parameter to ensure it only accepts 'date' or 'time' values.
protected function fieldFillDateTimeField(string $label, string $field, string $value): void {
+ // Validate that field is either 'date' or 'time'
+ if (!in_array($field, ['date', 'time'])) {
+ throw new \InvalidArgumentException(sprintf('Invalid field type "%s". Expected "date" or "time".', $field));
+ }
+
$xpath = sprintf(
'//*[self::span or self::label or self::h4][contains(normalize-space(.), "%s")]/ancestor::div[contains(@class,"form-item") or contains(@class,"field--widget-datetime-default")]//input[contains(@name,"[%s]")]',
$label,
$field
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Helper method to fill datetime field components. | |
| * | |
| * @param string $label | |
| * The field label. | |
| * @param string $field | |
| * The field component: 'date' or 'time'. | |
| * @param string $value | |
| * The value to set. | |
| * | |
| * @throws \Exception | |
| * If the field is not found. | |
| */ | |
| protected function fieldFillDateTimeField(string $label, string $field, string $value): void { | |
| $xpath = sprintf( | |
| '//*[self::span or self::label or self::h4][contains(normalize-space(.), "%s")]/ancestor::div[contains(@class,"form-item") or contains(@class,"field--widget-datetime-default")]//input[contains(@name,"[%s]")]', | |
| $label, | |
| $field | |
| ); | |
| $page = $this->getSession()->getPage(); | |
| $element = $page->find('xpath', $xpath); | |
| if (!$element) { | |
| throw new \Exception(sprintf('Date field with label "%s" value not found', $label)); | |
| } | |
| $element->setValue($value); | |
| } | |
| /** | |
| * Helper method to fill datetime field components. | |
| * | |
| * @param string $label | |
| * The field label. | |
| * @param string $field | |
| * The field component: 'date' or 'time'. | |
| * @param string $value | |
| * The value to set. | |
| * | |
| * @throws \Exception | |
| * If the field is not found. | |
| */ | |
| protected function fieldFillDateTimeField(string $label, string $field, string $value): void { | |
| // Validate that field is either 'date' or 'time' | |
| if (!in_array($field, ['date', 'time'])) { | |
| throw new \InvalidArgumentException( | |
| sprintf('Invalid field type "%s". Expected "date" or "time".', $field) | |
| ); | |
| } | |
| $xpath = sprintf( | |
| '//*[self::span or self::label or self::h4][contains(normalize-space(.), "%s")]/ancestor::div[contains(@class,"form-item") or contains(@class,"field--widget-datetime-default")]//input[contains(@name,"[%s]")]', | |
| $label, | |
| $field | |
| ); | |
| $page = $this->getSession()->getPage(); | |
| $element = $page->find('xpath', $xpath); | |
| if (!$element) { | |
| throw new \Exception(sprintf('Date field with label "%s" value not found', $label)); | |
| } | |
| $element->setValue($value); | |
| } |
🤖 Prompt for AI Agents
In src/FieldTrait.php between lines 238 and 264, the method
fieldFillDateTimeField accepts a $field parameter but does not validate its
value. Add validation to ensure $field is either 'date' or 'time' before
proceeding. If $field is invalid, throw an exception with a clear message. This
prevents incorrect usage and potential errors in XPath construction.
48f6c68 to
2316f40
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #409 +/- ##
==========================================
+ Coverage 80.25% 80.33% +0.07%
==========================================
Files 29 29
Lines 1965 1978 +13
==========================================
+ Hits 1577 1589 +12
- Misses 388 389 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
skipper-vp
added
Needs review
AlexSkrypnyk
added
Requires more work
AlexSkrypnyk
changed the title
#406
Summary by CodeRabbit