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.

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

CPS-356: Fix issue with End/Start date not updating on Peoples tab #714

Merged
merged 6 commits into from
Mar 2, 2021

Conversation

deb1990
Copy link

@deb1990 deb1990 commented Feb 24, 2021

Overview

In the peoples tab of Case Details, after updating the Start/End date from the inline datepicker, the dates got reverted when going to another tab, and then coming back to Peoples tab. This PR fixes this bug.

Before

before

After

after

Technical Details

Every time we go to Peoples tab, it fetches the Roles from $scope.item. But once we update a relationship, if we don't call refresh function, the $scope.item wont be changed. So again it will revert back to previous values, when going back to the tab.
But calling $scope.refresh updates the $scope.item object, which trickles down to individual tabs like People Tab.

For all other fields of the people tab, $scope.refresh was called, but it was not for while updating the dates, hence the bug happened.

To fix this, ang/civicase-base/directives/send-to-api-on-change.directive.js has been removed as its not used anymore.
Also replaced data-api-data="roleDatesUpdater.getApiCallsForStartDate(role, item.id)" with
ng-change="refresh(roleDatesUpdater.getApiCallsForStartDate(role, item.id))".

Backstop JS

Similar to compucorp/backstopjs-config#45, the ability to run backstop on customisable branches has been added. The following inputs are taken from the user to run the tests

  • CiviCase Reference Branch/Tag Name
  • CiviCase Test Branch/tag Name
  • CiviCRM Reference Version
  • CiviCRM Test Version
  • Shoreditch Reference Branch/Tag Name
  • Shoreditch Test Branch/Tag Name

Also all the backstop scenarios has been fixed, and in my last few testing, there was 0 false positives.
Also removed loading state scenarios as they were not working 90% of the time.

@deb1990 deb1990 added the bug Something isn't working label Feb 24, 2021
@deb1990 deb1990 force-pushed the CPS-356-end-date-issue branch from e5562c8 to caa75de Compare February 24, 2021 18:07
@deb1990 deb1990 force-pushed the CPS-356-end-date-issue branch 16 times, most recently from b91fd3c to 02d6910 Compare February 25, 2021 19:23
@deb1990 deb1990 force-pushed the CPS-356-end-date-issue branch from 02d6910 to 62e674e Compare February 26, 2021 03:50
@@ -22,7 +22,7 @@
"--no-sandbox"
]
},
"asyncCaptureLimit": 3,
"asyncCaptureLimit": 1,
Copy link
Member

Choose a reason for hiding this comment

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

why was this reduced?

Copy link
Author

@deb1990 deb1990 Mar 2, 2021

Choose a reason for hiding this comment

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

Using more than 1 thread creates lot of false negatives. But with 1, its much more reliable.

@@ -2,7 +2,7 @@

const Utility = require('./utility.js');

module.exports = async (page, scenario, viewport) => {
module.exports = async (page, scenario, viewport, dontWait) => {
Copy link
Member

Choose a reason for hiding this comment

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

too many parameters. Let's use options if possible. And instead of don't wait why not a { wait: 0 } with the default being 1000 ?

}

await mouseEventsHelper(page, scenario);
await mouseEventsHelper(page, scenario, null, true);
Copy link
Member

Choose a reason for hiding this comment

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

null, and true are not very clear. This is the problem of sending a lot of parameters, specially boolean parameters.

// remove drupal/civicrm error logging, which creates difference
await this.removeElements('#console');

await this.closeSystemErrorNotification();
Copy link
Member

Choose a reason for hiding this comment

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

why not remove the notifications instead of closing them and waiting for them to close?

@deb1990 deb1990 force-pushed the CPS-356-end-date-issue branch from f260db3 to 1f434ec Compare March 2, 2021 12:01
@deb1990 deb1990 merged commit f812bbc into master Mar 2, 2021
@deb1990 deb1990 deleted the CPS-356-end-date-issue branch March 2, 2021 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants