appsmith icon indicating copy to clipboard operation
appsmith copied to clipboard

fix:Unpredictable movements of the dates in the date picker included …

Open Harshithazemoso opened this issue 1 year ago • 19 comments

…in the table widget; are decreased by 1 day #25081

Description

[!TIP]
Add a TL;DR when the description is longer than 500 words or extremely technical (helps the content, marketing, and DevRel team).

Please also include relevant motivation and context. List any dependencies that are required for this change. Add links to Notion, Figma or any other documents that might be relevant to the PR.

Fixes #25081
or
Fixes https://github.com/appsmithorg/appsmith/issues/25081

[!WARNING]
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.

Automation

/ok-to-test tags=""

:mag: Cypress test results

[!CAUTION]
If you modify the content in this section, you are likely to disrupt the CI result for your PR.

Communication

Should the DevRel and Marketing teams inform users about this change?

  • [ ] Yes
  • [ ] No

SCREENSHOTS: Screenshot from 2024-05-31 17-52-19

Video:Fixing the issue

Summary by CodeRabbit

  • New Features

    • Introduced test cases for adding new rows and editing date columns in the table widget.
  • Bug Fixes

    • Improved the date selection logic in the table widget to ensure consistent and correct date formatting.
  • Chores

    • Added a utility function to retrieve the current date in ISO format with a specific timezone offset.

Harshithazemoso avatar May 31 '24 12:05 Harshithazemoso

Walkthrough

Recent updates include new test cases for adding and editing rows in a table widget, enhancements in date handling within the widget's date cells, and comprehensive Git utility modifications for application resource management. These changes aim to improve test coverage, code maintainability, and Git integration functionalities.

Changes

Files Summary
...ClientSide/Widgets/TableV2/AddNewRowEditColumn_spec.* Introduces test cases for adding rows and editing columns in a table widget, ensuring date manipulation works.
.../TableWidgetV2/component/cellComponents/DateCell.tsx Refactors date handling for row updates, enhancing code clarity and efficiency.
...Src/main/java/com/appsmith/server/helpers/ce/GitFileUtilsCE.java Refactors and extends methods for managing application resources in Git repositories, including new methods for handling themes, pages, and custom libraries.
.../cypress/support/Pages/DateHelper.ts Adds getCurrentDateISO function to return the current date in ISO format with timezone offset.

Sequence Diagram(s)

sequenceDiagram
    participant Tester as Cypress Test Suite
    participant UserApp as User Application
    participant TableWidget as Table Widget
    participant DateHandler as DateCell Component

    Tester->>UserApp: Initialize test
    UserApp->>TableWidget: Add new row
    TableWidget->>DateHandler: Edit date column
    DateHandler->>DateHandler: Format date
    DateHandler->>TableWidget: Update row with formatted date
    TableWidget->>UserApp: Reflect changes in UI
    UserApp->>Tester: Verify changes
sequenceDiagram
    participant App as Application
    participant GitUtils as GitFileUtilsCE
    participant Repo as Git Repository

    App->>GitUtils: Save application to local repository
    GitUtils->>GitUtils: Serialize application resources
    GitUtils->>Repo: Store serialized data
    Repo-->>App: Confirm save
    App->>GitUtils: Reconstruct application from repository
    GitUtils->>Repo: Fetch saved data
    GitUtils->>GitUtils: Deserialize application resources
    GitUtils-->>App: Provide reconstructed application data

Poem

In lines of code where logic flows,
Date cells dance in gentle prose.
New rows bloom in table's space,
With dates aligning, a seamless grace.
Git whispers, "Save and load,"
In every repo it has sowed.
Progress hums in every part,
As teamwork crafts a work of art.


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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 as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

coderabbitai[bot] avatar May 31 '24 12:05 coderabbitai[bot]

I would assume the changes in DateCell.tsx do sovle the issue. However I'm not quite sure your test is usefull though I do not know well that testing framework.

Does the test fail before the correction? That would be a good way to check if the test is actually appropriate. Hello @benjamindonze ,Yea I have checked with the earlier version that was showing a wrong date the test was failing ,later I have modified it and the test is working correctly ,you can verify the cypress testing in the provided link Video for cypress testing

Harshithazemoso avatar Jun 10 '24 08:06 Harshithazemoso

hello @benjamindonze ,can we merge it now ?

Harshithazemoso avatar Jun 10 '24 09:06 Harshithazemoso

Sure for me it seems ok

benjamindonze avatar Jun 10 '24 10:06 benjamindonze

Sure for me it seems ok

@benjamindonze but it seems the merging is still in blocked stage

Harshithazemoso avatar Jun 10 '24 10:06 Harshithazemoso

Well I don't know your processes for merging PR and so, personally I'm just a user that reported a bug I did approve the PR though I don't know if it has any effect as I'm have no specific access rights on this project

benjamindonze avatar Jun 10 '24 10:06 benjamindonze

hello @Nikhil-Nandagopal ,could you please review the pr

Harshithazemoso avatar Jun 10 '24 10:06 Harshithazemoso

@jsartisan please review this PR. Here is the internal PR

@Harshithazemoso you have written the test using JS ext, please use TS instead.

somangshu avatar Jun 17 '24 07:06 somangshu

@jsartisan please review this PR. Here is the internal PR

@Harshithazemoso you have written the test using JS ext, please use TS instead.

hello @somangshu all the specs are in js ext so I followed the same format ,should I update that ?

Harshithazemoso avatar Jun 18 '24 04:06 Harshithazemoso

I see an error in the build

Error: There's new test files in JS
🔴 There's new test files in JS, please port to TS and re-trigger Cypress tests:
1. app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/AddNewRowEditColumn_spec.js

@jsartisan isint that a problem?

somangshu avatar Jun 18 '24 08:06 somangshu

@somangshu Seems like there is some check that requires new cypress tests to be in typescript.

@Harshithazemoso Can we write the test in typescript? It will help us pass the build check

jsartisan avatar Jun 18 '24 10:06 jsartisan

@somangshu Seems like there is some check that requires new cypress tests to be in typescript.

@Harshithazemoso Can we write the test in typescript? It will help us pass the build check

@jsartisan made the changes can you review it now

Harshithazemoso avatar Jun 18 '24 14:06 Harshithazemoso

Few improvements needed on cypress spec -

  1. Use variables or constants for data and selectors to improve maintainability and readability. ex - Please use [data-testid="t--property-pane-back-btn"] from common locators ie editPropBackButton _addNewRow instead of .t--add-new-row from app/client/cypress/support/Pages/Table.ts
  2. Use updated helpers from aggregateHelpers instead on get and click use GetNClick
  3. Move date logic into helper functions or utilities to keep test code clean and focused on test logic.
  4. Write description explaining the spec and what kind of scenario is being tested for more readability
  5. Please use better locators wherever possible. ex cy.get(".DayPicker-Day--today > .bp3-datepicker-day-wrapper").click();). Use parent or some other locator to click
  6. Remove commented code

hello @ApekshaBhosale made the changes ,can you please review it now

Harshithazemoso avatar Jun 19 '24 08:06 Harshithazemoso

@Harshithazemoso the test that you have written has failed.

AssertionError: Timed out retrying after 60000ms: Expected to find element: `//div[contains(@class,'t--property-control-allowaddingarow')]//input[@type='checkbox'] | //label[text()='Allow adding a row']//input[@type='checkbox']`, but never found it.
    at AggregateHelper.GetElement (cypress/support/Pages/AggregateHelper.ts:256:26)
    at PropertyPane.TogglePropertyState (cypress/support/Pages/PropertyPane.ts:199:17)
    at Context.eval (cypress/e2e/Regression/ClientSide/Widgets/TableV2/AddNewRowEditColumn_spec.ts:32:19)

Cypress Dashboard 22508437 6347065 1

cc @jsartisan

somangshu avatar Jun 20 '24 06:06 somangshu

@Harshithazemoso the test that you have written has failed.

AssertionError: Timed out retrying after 60000ms: Expected to find element: `//div[contains(@class,'t--property-control-allowaddingarow')]//input[@type='checkbox'] | //label[text()='Allow adding a row']//input[@type='checkbox']`, but never found it.
    at AggregateHelper.GetElement (cypress/support/Pages/AggregateHelper.ts:256:26)
    at PropertyPane.TogglePropertyState (cypress/support/Pages/PropertyPane.ts:199:17)
    at Context.eval (cypress/e2e/Regression/ClientSide/Widgets/TableV2/AddNewRowEditColumn_spec.ts:32:19)

Cypress Dashboard 22508437 6347065 1

cc @jsartisan

@somangshu can you check now

Harshithazemoso avatar Jun 20 '24 06:06 Harshithazemoso

@ApekshaBhosale if you give an approval, I run the test internally. Or let me know if you want me to run it before you can review.

somangshu avatar Jun 20 '24 06:06 somangshu

@ApekshaBhosale if you give an approval, I run the test internally. Or let me know if you want me to run it before you can review.

hello @somangshu its been more than a week there is no update about the pr review

Harshithazemoso avatar Jun 26 '24 08:06 Harshithazemoso

@ApekshaBhosale please check this out!

somangshu avatar Jun 26 '24 08:06 somangshu

@nidhi-nair ,@somangshu can you please assign a reviewer for this ticket

Harshithazemoso avatar Jul 02 '24 04:07 Harshithazemoso

@Harshithazemoso apologies, the test still fails with the same error on internal test run. We will have to close this PR now because the change seems to be more involved.

somangshu avatar Jul 02 '24 09:07 somangshu

hange seems to be more involved.

Hello @somangshu , Thank you for letting me know about the test results. Could you please provide more details about the specific issue causing the test to fail,The test cases are running fine in my local and understanding the root cause will help me address it more effectively. Additionally, I would like to know more about why the PR needs to be closed and any next steps we should consider or are there any improvements that needed to be taken care of ?

Harshithazemoso avatar Jul 02 '24 11:07 Harshithazemoso

@Harshithazemoso its the same cypress failure as before.

somangshu avatar Jul 03 '24 10:07 somangshu

@Harshithazemoso its the same cypress failure as before.

@somangshu yeah you have mentioned about this change seems to be more involved right ,Can I know additional details about this ?

Harshithazemoso avatar Jul 03 '24 10:07 Harshithazemoso

Since the test keep failing, to understand what is happening we need to debug this internally. We know that there is some difference that comes up in local vs github action run. We do not have the bandwidth to pick this up right now and you dont have the right permissions to check this throughly, hence decided to close it for the time being.

somangshu avatar Jul 03 '24 10:07 somangshu

Since the test keep failing, to understand what is happening we need to debug this internally. We know that there is some difference that comes up in local vs github action run. We do not have the bandwidth to pick this up right now and you dont have the right permissions to check this throughly, hence decided to close it for the time being.

Okay ,Somangshu Thanks for your response

Harshithazemoso avatar Jul 03 '24 10:07 Harshithazemoso

Does it all mean that the issue about the dates won't be fixed now?

benjamindonze avatar Jul 03 '24 10:07 benjamindonze

Right now there is a workaround available.

We are currently in the process to add a team to address widget based issues, it can be prioritised then.

somangshu avatar Jul 03 '24 11:07 somangshu