I think the best would be, if you are using the node status to determine, if a added holiday should be enabled or not.
To enable a newly added holiday by default edit the storm_holidays_enabled variable
Something like the following code on node update/insert

$enabled = variable_get('storm_holidays_enabled', array());
$enabled[$holiday_title] = $holiday_title; // or 0 to disable it
variable_set('storm_holidays_enabled', $enabled);

Comments

d34dman’s picture

d34dman commented

I would include this feature in my node update and insert. But when i look at the function i feel there is an improvement required.

What if there are multiple holidays with same name?

I know i sound silly. But in case if we are not allowing user to enter same title for different holidays, then we need to explicitly make them aware somehow. A message while entering title, suggesting them to keep it unique or something. And also validating for unique title for holidays in the same year while node insert/update.

Sounds like the effort that we would require to implement unique title restriction is more than it would be to allow similar title.

Like replacing $enabled[$holiday_title] by $enabled[$nid] or $enabled[$holiday_title . $nid].

How should Storm_OHM cohabit with currently selected country holidays?

  1. Should it extend the holidays of the country that is selected in Storm Contrib's Holiday Manager?
  2. Should it be independent and only come in to effect when Storm Organization Holiday Manager is selected as a country in Storm Contrib's admin page?

Case 1: If Storm OHM extends Holiday Manager. This means we will have two places where we update storm holiday lists. One from the node form and another from the Storm Contrib admin settings form.

Case 2: If Storm OHM is independent. Right now Storm OHM behaves like a country and list all the nodes as holidays list available to be turned on/ off in the storm contrib admin settings form.

This approach also doesn't solve the UX issue of having to check two different places to manage the same thing.

Proposed Solution

The following code,

<?php
$enabled = variable_get('storm_holidays_enabled', array());
$enabled[$holiday_title] = $holiday_title; // or 0 to disable it
variable_set('storm_holidays_enabled', $enabled);
?>

should actually be modified to something like,

<?php
//NOTE: VARIABLE NAMES ARE FOR SUGGESTIVE PURPOSE ONLY.
function storm_ohm_update_storm_holidays() {
  $enabled = array();
  if( storm_ohm_merge_with_current_country == TRUE){
    $enabled = variable_get('storm_holidays_enabled', array());
  }
  if( storm_holidays_country == 'Storm_OHM'){
    foreach (get_storm_ohm_nodes() as $temp_node){
      $enabled[$temp_node->holiday_title] = $temp_node->holiday_title; // or 0 to disable it
    }
    variable_set('storm_holidays_enabled', $enabled);
  }
}
?>

And maybe the above function storm_ohm_update_storm_holidays() is called during storm_ohm node update/ insert / storm_contrib admin form submit.

d34dman’s picture

d34dman commented

the feature has been implemented.
Git checkout command.

git clone --branch 6.x-1.0-alpha1 http://git.drupal.org/sandbox/D34dMan/1309636.git storm_ohm

d34dman’s picture

d34dman commented
Status: Active » Needs review
kfritsche’s picture

kfritsche
Primary language German
Location 🇩🇪🇪🇺
commented
Status: Needs review » Needs work

Like replacing $enabled[$holiday_title] by $enabled[$nid] or $enabled[$holiday_title . $nid].

I can't change this in storm_holidays, because a normal country holiday module, do not have node ids. We used the title there, because holidays were added by code and that there aren't any holidays with the same name.
But in this case, you are right, the user can do this. There should be an validation for that case.
I have something like that in mind: (I didn't test it right now!)

function storm_ohm_validate($node, &$form) {
  $count = db_result(db_query("SELECT COUNT(nid) FROM storm_ohm stohm JOIN node n ON (n.vid = stohm.vid) WHERE n.title ='%s' AND holiday_year = %d", $node->title, format_date($node->holiday_date, 'custom', 'Y')));
  if ($count > 0) {
    form_set_error('title', t('Holiday @name already exists in @year', array('@name' => $node->title, '@year' => format_date($node->holiday_date, 'custom', 'Y'))))
  }
}

So it is possible to have same holidays in different years.
The rest seems to be working for this issue. Further discussion about OHM merging other Country Holidays should be moved to the corresponding issue (#1310660: Add OHM Holidays to Country Holidays)