It would be good tone to handle the case when $feed_element contains empty strings to not produce the wrong results on import.
If the $sub_field is equal to 'hours' and $feed_element contains an empty string this produces the 0 values for 'start_hour' and 'end_hour' which leads to round-the-clock values for office_hour field.
Any thoughts/objections against that idea?

Thanks in advance!

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ruslan.muradov’s picture

In case if anybody interested in solving this minor issue here is the patch that I have made. Hope it will helps

ruslan.muradov’s picture

Here is the "clean" version of patch - trailing whitespace is removed and the comments are compliant to drupal.org standards

johnv’s picture

Status: Active » Needs review

Thanks for the patch. Correcting status.
Be aware that the patch seems to be implemented on top of #1965966: On feeds import, wrong deltas when entity has multiple 'office_hours'-fields.

ruslan.muradov’s picture

Be aware that the patch seems to be implemented on top of #1965966: Wrong deltas in 'office_hour' field on feeds import

Hmm. Not sure what you mean..
I have made the patch against the "clean" version of office_hours.feeds.inc file which I have obtained from 7.x-dev branch of project git-repo.
Cheers.

johnv’s picture

Yes, you are right, I was confused with the other patch.

This patch contains a 'return '. Which is quiet confusing for the quick reader.
Can you change it to a 'normal if-then-else?

ruslan.muradov’s picture

Can you change it to a 'normal if-then-else?

I was decided to use if() clause with return statement to keep the code as clean as possible. it-then-else produce less clean code in that case. But this is only IMO. Of course you can rewrite the code in any way you think is needed!
Thanks for the support and quick responses! Cheers!

johnv’s picture

Title: Handle the empty values in office_hours_feeds_set_target() to not produce wrong results on feeds import » Handle the exceptional values in office_hours_feeds_set_target()
Category: feature » bug
Status: Needs review » Needs work

This needs some more tests, and add more exeptions .
What happens (before and after this patch) if the following is imported:
- 09:00 - 19:00 (normal situation)
- - (this patch ? )
- 00:00 - 00:00 (this patch ? )
- 00:00 - 24:00 (around-the-clock: for now: 24:00 must be stored as 00:00, which is corrected in formatter)
- - 18:00 (error message?)
- 18:00 - (error message?)

ruslan.muradov’s picture

What happens (before and after this patch) if the following is imported

You are right! There are a lot of cases when module does not handle the edge case conditions.
In that case there is three options
1) the user must control the correctness of incoming data
2) the module should contain most robust code which will guard against the data integrity
3) handle the cases through custom tampers

I have achieved correct behavior using the 3-rd way.

johnv’s picture

Category: Bug report » Feature request
Issue summary: View changes

For better statistics, let's call this a feature request :-)

johnv’s picture

Component: Code » Feeds integration
johnv’s picture

Component: Feeds integration » Code
johnv’s picture

Component: Code » Integrating Feeds
johnv’s picture

Version: 7.x-1.1 » 7.x-1.x-dev
Status: Needs work » Closed (outdated)

Given the life cycle of D7, this issue is considered closed. However, patches are appreciated.