Author Topic: Make Expired Relationship Handling Consistent (Read 1025 times)

Michael Z Daryabeygi · May 18, 2016, 08:26:14 am

It appears that the feature of representing a Relationship as expired has been implemented in an ad hoc manner.
There are problems of inconsistent nomenclature, insufficient contextual help, and duplicative and inconsistent business logic.

The Relationship tab has heading for "Inactive Relationships" and a helpful description, "These relationships are Disabled OR have a past End Date."

In fact, the "Enabled" status is called is_active in the database and the API. This is not noted in the interface.
"Active" seems more appropriate for a relationship as relationships are not "Enabled" and "Disabled" in the real world.

Although the relationship tab implements the business logic to represent an "Inactive" relationship, the state is not persisted on the record.
Instead, a scheduled job that is not enabled by default must do the work. Having this as a scheduled job seems like a fine optimization but the status of it being disabled is not easily stumbled upon because the interface operates fine without it.

Some potential changes:
- Make "Enabled/Disabled" consistent with data and api by changing with "Active/Inactive"
- When editing a relationship, if an end date of today or in the past is added and the scheduled job is disabled, prompt the user with:
"The scheduled job "Disable Expired Relationships" is not enabled." Would you like to make this relationship inactive?"
- Have the tab display only respect the is_active status. If an expired relationship that is active exists and there are no inactive relationships, display a message: The scheduled job "Disable Expired Relationships" is not enabled.

Please help improve this proposal.
This is an iteration of my initial proposal filed in Jira:
https://issues.civicrm.org/jira/browse/CRM-18561

Dave Doligalski · May 18, 2016, 08:51:38 am

Michael, nice suggestions.

Bullet point 1: Yes, consistency and appropriateness rule!!!

Bullet point 2: I would suggest, "The scheduled job "Disable Expired Relationships" is not enabled." not be displayed. Or perhaps only include it if the current user has some admin level permission in CiviCRM. A non-admin level civi user would only be confused with that message.

Actually, at the time of submission of the edit, set the Active/Inactive flag automatically? Why ask? Perhaps a status message might notify the user if the status has changed.

Bullet point 3: I think making the tab display default to "is_active" makes sense. Just so there are filtering options to display all too. Again, the displayed message is confusing to non-admins and probably should not be displayed. (Or reworded??)

andrewhunt · May 18, 2016, 08:52:55 am

This is a frustrating feature area to be sure--users are definitely given the impression that a past end date is the same as being inactive.

I would also add a situation: searching for relationships. When searching for relationships in Advanced Search, ones with past end dates and ones that are not enabled are treated the same: they appear when you search for "inactive" relationships but not when you search for active ones. However, when you search for related contacts, ones related via a past-end-date relationship appear alongside active ones.

Re: your proposed changes:
1. I see the rationale, but plenty of other things have "enabled" in the UI but is_active in the database and API
2. This makes a lot of sense. The wording might need a little bit of improvement, but the gist is perfect.
3. I like this in general, but it will be a big change for legacy organizations. If this is implemented, there will need to be more help text there, and the upgrade messages should emphasize the value of at least running the disable expired relationships job at least once.

nicolas · May 18, 2016, 08:57:26 am

How about:
- setting the flag to inactive on save if end date is past (rather than ask the user about it)
- enabling the scheduled job by default, and not allowing to disable it (this is a system job, why would anyone disable it? It also has no performance impact)

This seems like an easier way of dealing with this. Neutral about the column name change in the DB - does not seem to add anything but naming consistency.

JoeMurray · May 18, 2016, 09:22:35 am

Quote

Make "Enabled/Disabled" consistent with data and api by changing with "Active/Inactive"

There are many areas where db fields have different names in the interface. And pervasively throughout the codebase Enabled? values are stored in fields titled is_active, with two not quite exceptions in cases where an object already has an is_active field:
./schema/Contribute/ContributionPage.xml: <name>is_confirm_enabled</name>
./schema/PCP/PCPBlock.xml: <name>is_tellfriend_enabled</name>

So I would not favour breaking the pattern in UI of using Enabled or the pattern in the schema of using is_active unless the change was made everywhere. Consistency is more important.

Quote

When editing a relationship, if an end date of today or in the past is added and the scheduled job is disabled, prompt the user with:
"The scheduled job "Disable Expired Relationships" is not enabled." Would you like to make this relationship inactive?"

Wouldn't it be best to enable the cron job by default? As a second best, how about asking at this point if they would like to enable the cron job and doing that for them? In fact if you want to do a workaround for the cron job not being enabled, I would prompt whenever a user enters a value, not limiting as you propose. The reason is that it would solve the problem more generally, including for relationships with end dates set in other ways and for cases where the end date is set when the relationship is created, like for an appointment to a board of directors. This approach would 'smell' better to me than the suggested partial fix.

If you don't like these ideas, can you expand on what you mean by 'Although the relationship tab implements the business logic to represent an "Inactive" relationship, the state is not persisted on the record.' What is persisted to db that the cron job uses to update the status of enabled/active?

Quote

Have the tab display only respect the is_active status. If an expired relationship that is active exists and there are no inactive relationships, display a message: The scheduled job "Disable Expired Relationships" is not enabled.

Again, this doesn't feel like a good work-around, adding code that doesn't address and solve the real problem. I'd suggest a) enabling the job for them as before. There is another use case as well where the job might be enabled, but someone just saved through UI an end-date in the past and the job hasn't yet run. To fix this, I would change code on update from UI to set is_active appropriately based on enddate<now();

Sorry, I'm not trying to be a curmudgeon, but my sensibilities of the best approach are different here. I can see why you wanted feedback - it feels yucky.

jamien-www.compucorp.co.uk · May 18, 2016, 10:07:55 am

Hi guys,

We use the relationships quite heavily in a number of instances and I'm really pleased that you've brought this up (and a little bit embarrassed that I hadn't done something about this sooner!).

I think schema change should be avoided as this will break existing code/extensions/views integration (and API calls?) that CiviHR relies on so if we can not do that I would appreciate it!

I think the other changes sounds good.

To summarise:

- If update or create relationship through UI with end date in the past (or start date in the future!) should automatically set "is inactive/is disabled" = 1.
- New notification on save that ^above takes place
- New cronjob to check end date in the past (or start date in the future!) and to update "is inactive/is disabled" = 1 appropriately. (Need to be careful that the cron job should not make "enabled" relationships that have been disabled without an end date or a start date being entered.)
- Relationship UI should only reflect the "is inactive/is disabled" = 1 property (or should it still do both this and put those with end date in past and start date in future in the "inactive section"??).
- Enable cronjob by default and/or prompt users to enable when viewing relationship tab if have "administer civicrm" permission?
- Notify users on CiviCRM update that this new job exists and that it has been enabled.
- Add to Tims new notification thingy for Civi 4.7+?
- Document this somewhere and add to book!!

This actually brings up a more general point around "end dates" that we are discussing with a client. Quite a lot of entities would benefit having an "end date" (addresses, emails, phones etc) that will be disabled when the appropriate date occurs (person phones up as they are moving house in 2 weeks time and would like their publication delivered to the new address).

The general idea is the same as above but I would suggest that the table is more of "entity ID | entity type | start date | end date" and then the code updates the entity appropriately. For things like the (Phone/addresses/email) etc we were thinking that this could then be put into a log table and perhaps in a future version we could provide a UI with a restore feature. Maybe this is all a good opportunity for leap(/trip over/get up again and try a bit harder) by extension?

We are scoping this feature with them. Would it make sense perhaps to combine the mechanics for this and relationships? Please ping me (jamie@compucorp.co.uk) if you think worthwhile.

Best

Jamie

Michael Z Daryabeygi · May 18, 2016, 03:32:17 pm

@JMA - RE "not persisted to the db"

What I meant was... with no change to the record, i.e., a previously entered end date slips into the past... then the interface has business logic that affects the relationship being displayed as "Inactive" but the "is_active" field is unchanged. I'm saying this logic was built into the wrong place.

RE - enabling the cron job by default

This would not have any impact on upgraded instances... I don't think. But I guess an upgrade step could be added to enable it. It might be best to change the default for new installs and add the other features to accommodate upgraded instances and leave their configurations alone. I might be being too conservative though and little to no harm could come of enabling the job in the upgrade process.

Michael Z Daryabeygi · May 18, 2016, 03:43:37 pm

Thanks for the input everyone!

This is some good stuff.

I'll try to rope in the core team and get some input on where to go from here.

davej · May 20, 2016, 02:13:09 am

I agree with nicolas' approach...

Quote from: nicolas on May 18, 2016, 08:57:26 am

How about:
- setting the flag to inactive on save if end date is past (rather than ask the user about it)
- enabling the scheduled job by default, and not allowing to disable it (this is a system job, why would anyone disable it? It also has no performance impact)

This seems like an easier way of dealing with this. Neutral about the column name change in the DB - does not seem to add anything but naming consistency.

... though I don't feel the need to prevent disabling the scheduled job.

CiviCRM Community Forums (archive)

News:

Author Topic: Make Expired Relationship Handling Consistent (Read 1025 times)

Michael Z Daryabeygi

Make Expired Relationship Handling Consistent

Dave Doligalski

Re: Make Expired Relationship Handling Consistent

andrewhunt

Re: Make Expired Relationship Handling Consistent

nicolas

Re: Make Expired Relationship Handling Consistent

JoeMurray

Re: Make Expired Relationship Handling Consistent

jamien-www.compucorp.co.uk

Re: Make Expired Relationship Handling Consistent

Michael Z Daryabeygi

Re: Make Expired Relationship Handling Consistent

Michael Z Daryabeygi

Re: Make Expired Relationship Handling Consistent

davej

Re: Make Expired Relationship Handling Consistent