MVHub

Merge lp:~felmas/mvhub/contact_2nf_dialect into lp:mvhub

  1. contact_2nf_dialect
  2. Merge into trunk_2format
Proposed by Ferhat Elmas
Status: Needs review
Proposed branch: lp:~felmas/mvhub/contact_2nf_dialect
Merge into: lp:mvhub
Diff against target: 448 lines (+149/-146)
5 files modified
app-mvhub/conf/sql_select.lib (+1/-0)
app-mvhub/conf/sql_update.lib (+1/-2)
app-mvhub/conf/templates/html/call_manager.tmpl (+36/-43)
lib-mvhub/lib/MVHub/CGIAppBase.pm (+0/-2)
lib-mvhub/lib/MVHub/CallManager.pm (+111/-99)
To merge this branch: bzr merge lp:~felmas/mvhub/contact_2nf_dialect
Reviewer Review Type Date Requested Status
Dan MacNeil Needs Fixing
Review via email: mp+91529@code.launchpad.net

Description of the change

   Fixes update anomaly in call manager

To post a comment you must log in.
Revision history for this message
Dan MacNeil (omacneil) wrote :

423 - Params::Validate::validate_pos( @_, 1, 1 );
424 - my ( $record_href, $quick_login_url ) = @_;
425 +
426 + Params::Validate::validate_pos( @_, 1, 1, 1 );
427 + my $record_href = shift;
428 + my $quick_login_url = shift;
429 + my $expired_records_scalar = shift;

The book, Perl Best Practices says to handle sub routine parameters like on line 424. Arguably using more lines makes it easier to read, but the standard we have is to follow PBP .

Consistency is important.

A bit small for a 'needs fixing', but as a general rule changing existing formating is a small bit of a faux pas.

review: Needs Fixing
Reply
Revision history for this message
Dan MacNeil (omacneil) wrote :

Please revert all changes to

 sub _make_mailto_link

http://wiki.thecsl.org/mediawiki/index.php/Standards:Code_style_guide#Esthetics

As near as I can tell, I like my personal taste better than yours. :->

review: Needs Fixing
Reply
Revision history for this message
Dan MacNeil (omacneil) wrote :

56 <!--TMPL_LOOP NAME = 'CHECKBOX_LOOP_TAG' -->

maybe could be renamed since there are no checkboxes involved

Reply
Revision history for this message
Dan MacNeil (omacneil) wrote :

changing record_type from 'a' and 'p' to agency and program was an improvement

Reply
Revision history for this message
Dan MacNeil (omacneil) wrote :

252 + elsif ( $record_type eq 'agency' ) {
253 + push( @bind_values,

...if there is no (main agency record) needing an update, then it looks like the notes field may not get updated

Reply
Revision history for this message
Dan MacNeil (omacneil) wrote :

this is probably a bug before this branch,

If the agency doesn't need to be updated then
the program records that need to be updated don't get displayed in call manager.

To reproduce: update the main agency record for "Action Inc" in nsp.$USER.testing123.net

None of the program records, all out of date, all with 5 reminders will display in call manager

Reply
Revision history for this message
Dan MacNeil (omacneil) wrote :

> this is probably a bug before this branch,

actually, no same database, call manager in trunk, Program names of Action are present in call manager

Reply
Revision history for this message
Ferhat Elmas (felmas) wrote :

> 252 + elsif ( $record_type eq 'agency' ) {
> 253 + push( @bind_values,
>
> ...if there is no (main agency record) needing an update, then it looks like
> the notes field may not get updated

  Definitely, because when I coupled notes field for each record, you had said we don't need it, we can connect notes to agency.

Reply
Revision history for this message
Ferhat Elmas (felmas) wrote :

> If the agency doesn't need to be updated then
> the program records that need to be updated don't get displayed in call
> manager.

  In the expired records query, programs are selected according to their last_updated field, so this shouldn't be the case.

> To reproduce: update the main agency record for "Action Inc" in
> nsp.$USER.testing123.net
>
> None of the program records, all out of date, all with 5 reminders will
> display in call manager

  I have checked the table for program, as I see last_updated isn't an auto-update column and we aren't updating it anywhere, only name, last name, email and phone are updated. Therefore, some problems about last_updated seemed normal to me now.

  Could you rewrite how the functionality SHOULD be implemented, please? Since a bit time has passed, I am not sure what to do. However, I want to remind you that when we click update all all programs and agency records will be updated with current values in the text boxes? Therefore, what I want is which operation must be done on last_updated and reminders_sent here.

Reply

Unmerged revisions

602. By Ferhat Elmas <<email address hidden>>

fixed regex counter in Call_Manager.pm

601. By Ferhat Elmas <<email address hidden>>

fixed typo

600. By Ferhat Elmas <<email address hidden>>

modified program update query

599. By Ferhat Elmas <<email address hidden>>

added for loop to delete extra admin_notes field from program hashes

598. By Ferhat Elmas <<email address hidden>>

minor optimization in if check of update part of call manager

597. By Ferhat Elmas <<email address hidden>>

removed an extra select query

596. By Ferhat Elmas <<email address hidden>>

removed checkbox and extra notes from call manager

595. By Ferhat Elmas <<email address hidden>>

merged trunk

594. By Ferhat Elmas <<email address hidden>>

merged trunk

593. By Ferhat Elmas <<email address hidden>>

merged trunk

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'app-mvhub/conf/sql_select.lib'
2--- app-mvhub/conf/sql_select.lib 2011-12-02 15:08:42 +0000
3+++ app-mvhub/conf/sql_select.lib 2012-02-04 02:12:19 +0000
4@@ -919,6 +919,7 @@
5 a.agency_id=p.agency_id
6 AND p.last_updated < ?
7 AND p.reminders_sent < ?
8+
9 [PROGRAM_X_HAS_NEWSLETTER]
10 SELECT a.agency_name, p.program_name,
11 p.email, p.main_phone, p.news_letter_description
12
13=== modified file 'app-mvhub/conf/sql_update.lib'
14--- app-mvhub/conf/sql_update.lib 2011-12-02 15:08:42 +0000
15+++ app-mvhub/conf/sql_update.lib 2012-02-04 02:12:19 +0000
16@@ -62,8 +62,7 @@
17 contact_phone = ?,
18 contact_email = ?,
19 contact_first_name = ?,
20- contact_last_name = ?,
21- admin_notes = ?
22+ contact_last_name = ?
23 WHERE
24 program_id = ?
25
26
27=== modified file 'app-mvhub/conf/templates/html/call_manager.tmpl'
28--- app-mvhub/conf/templates/html/call_manager.tmpl 2011-12-02 15:08:42 +0000
29+++ app-mvhub/conf/templates/html/call_manager.tmpl 2012-02-04 02:12:19 +0000
30@@ -36,56 +36,46 @@
31 <!--TMPL_VAR NAME='agency_name' --> / <!--TMPL_VAR NAME='main_phone' -->
32
33 </td>
34+</tr>
35+<tr>
36+ <td>
37+
38+ <!--TMPL_VAR NAME='mailto' -->
39+ </td>
40 </tr>
41+
42 <tr>
43 <td>
44 <input type="hidden"
45 name="rm"
46 value="update_contact_info"
47 />
48- <input type="hidden"
49- name="agency_id"
50- value="<!--TMPL_VAR NAME='agency_id' -->"
51- />
52- Only CHECKED Program names are updated
53 </td>
54
55 </tr>
56 <!--TMPL_LOOP NAME = 'CHECKBOX_LOOP_TAG' -->
57 <tr>
58- <td>
59-
60- </td>
61 <td align="left">
62- <input type="checkbox"
63- name= "<!--TMPL_VAR NAME='checkbox_name' -->"
64- value="1"
65- checked="yes"
66- />
67- <!--TMPL_VAR NAME='record_name' -->
68- </td>
69- </tr>
70- <tr>
71+ <!--TMPL_VAR NAME='record_name' -->
72+ </td>
73 <td>
74-
75- </td>
76- <td> &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;
77-
78- last update:
79-
80- <!--TMPL_VAR NAME='last_updated' --> &nbsp;
81-
82- reminders sent:
83-
84- <!--TMPL_VAR NAME='reminders_sent' -->
85- </td>
86+ <input type="hidden"
87+ name='record_type_id_<!--TMPL_VAR NAME="__counter__" -->'
88+ value="<!--TMPL_VAR NAME='record_type_and_id' -->"
89+ />
90+ </td>
91 </tr>
92- <!--/TMPL_LOOP-->
93+ <tr>
94+ <td>
95+ last update: <!--TMPL_VAR NAME='last_updated' --> &nbsp;&nbsp;
96+ reminders sent: <!--TMPL_VAR NAME='reminders_sent' -->
97+ </td>
98+ </tr>
99 <tr>
100 <td align="left">
101 contact phone:
102 <input type="text"
103- name="contact_phone"
104+ name='contact_phone_<!--TMPL_VAR NAME="__counter__" -->'
105 size="12"
106 value="<!--TMPL_VAR NAME='contact_phone' -->"
107 />
108@@ -93,7 +83,7 @@
109 <td align="left">
110 contact email:
111 <input type="text"
112- name="contact_email"
113+ name='contact_email_<!--TMPL_VAR NAME="__counter__" -->'
114 size="12"
115 value="<!--TMPL_VAR NAME='contact_email' -->"
116 />
117@@ -103,7 +93,7 @@
118 <td align="left">
119 contact first:
120 <input type="text"
121- name="contact_first_name"
122+ name='contact_first_name_<!--TMPL_VAR NAME="__counter__" -->'
123 size="12"
124 value="<!--TMPL_VAR NAME='contact_first_name' -->"
125 />
126@@ -111,23 +101,26 @@
127 <td align="left">
128 contact last:
129 <input type="text"
130- name="contact_last_name"
131+ name='contact_last_name_<!--TMPL_VAR NAME="__counter__" -->'
132 size="12"
133 value="<!--TMPL_VAR NAME='contact_last_name' -->"
134 />
135 </td>
136 </tr>
137+ <tr>
138+ <td>
139+ <!--TMPL_IF NAME="__first__" -->
140+ <textarea name='admin_notes_<!--TMPL_VAR NAME="__counter__" -->' rows="7" cols="30">
141+ <!--TMPL_VAR NAME='admin_notes' -->
142+ </textarea>
143+ <!--/TMPL_IF-->
144+ </td>
145+ </tr>
146+ <!--/TMPL_LOOP-->
147 <tr>
148 <td>
149- <!--TMPL_VAR NAME='mailto' -->
150- </td>
151- </tr>
152- <tr>
153- <td>
154- <textarea name="admin_notes" rows="7" cols="30">
155- <!--TMPL_VAR NAME='admin_notes' -->
156- </textarea>
157- </td>
158+
159+ </td>
160 <td align="center" valign ="middle">
161 <input type="submit"
162 name="submit"
163
164=== modified file 'lib-mvhub/lib/MVHub/CGIAppBase.pm'
165--- lib-mvhub/lib/MVHub/CGIAppBase.pm 2011-12-02 15:08:42 +0000
166+++ lib-mvhub/lib/MVHub/CGIAppBase.pm 2012-02-04 02:12:19 +0000
167@@ -5,8 +5,6 @@
168 # PROJECT: https://launchpad.net/mvhub/
169
170 # PURPOSE: MEANINGFUL PURPOSE IS REQUIRED!!
171-our ($VERSION) = '$Revision: 1730 $' =~ /([.\d]+)/;
172-
173 package MVHub::CGIAppBase;
174
175 use strict;
176
177=== modified file 'lib-mvhub/lib/MVHub/CallManager.pm'
178--- lib-mvhub/lib/MVHub/CallManager.pm 2011-12-02 15:08:42 +0000
179+++ lib-mvhub/lib/MVHub/CallManager.pm 2012-02-04 02:12:19 +0000
180@@ -36,54 +36,52 @@
181
182 my $self = shift;
183
184- my %notifications = _get_call_notifications($self);
185-
186- my $call_recs_aref = _make_call_records_from( $self, %notifications );
187+ my $call = _get_call_notifications($self);
188
189 my $template = $self->load_tmpl(
190 'call_manager.tmpl',
191 global_vars => 1, # for is_admin
192 loop_context_vars => 1 # for coloring every other program row
193 );
194- $template->param( CALL_RECORDS_LOOP_TAG => $call_recs_aref );
195+ $template->param( CALL_RECORDS_LOOP_TAG => $call );
196
197- # $template->param(CALL_RECORDS_LOOP_TAG => \@{$all_calls{unique_id}});
198 return $template->output();
199 }
200
201 sub update_contact_info {
202 my $self = shift;
203
204- my %contact_form_values = $self->query->Vars();
205- my %cf = %contact_form_values;
206- my @bind_values = (
207- $cf{contact_phone}, $cf{contact_email},
208- $cf{contact_first_name}, $cf{contact_last_name},
209- $cf{admin_notes}
210- );
211+ my %cf = $self->query->Vars();
212
213- my @checkbox_names = grep( /checkbox/, keys(%contact_form_values) );
214+ my @record_type_ids = grep( /record_type_id/, keys(%cf) );
215
216 my $program_update_sql = $self->retr_update_sql('PROGRAM_X_CONTACT');
217 my $agency_update_sql = $self->retr_update_sql('AGENCY_X_CONTACT');
218 my $dbh = $self->dbh();
219
220- foreach my $checkbox_name (@checkbox_names) {
221-
222- # only checked checkboxes ie value==1
223- next if !$cf{$checkbox_name};
224-
225- my ( $record_id, $record_type )
226- = ( $checkbox_name =~ m /^(\d+)_(a|p)/ );
227+ foreach my $record_type_id (@record_type_ids) {
228+
229+ my ($counter) = $record_type_id =~ m /^record_type_id_(\d+)$/;
230+ my ( $record_type, $record_id )
231+ = ( $cf{$record_type_id} =~ m /^(program|agency)_(\d+)$/ );
232+
233+ my @bind_values = (
234+ $cf{"contact_phone_$counter"},
235+ $cf{"contact_email_$counter"},
236+ $cf{"contact_first_name_$counter"},
237+ $cf{"contact_last_name_$counter"}
238+ );
239+
240 MVHub::Utils::assert( defined $record_id,
241- "'$checkbox_name' regex failed to match record_id" );
242+ "'$record_type_id' regex failed to match record_id" );
243 MVHub::Utils::assert( defined $record_type,
244- "for '$checkbox_name' regex failed to match record_type" );
245+ "for '$record_type_id' regex failed to match record_type" );
246
247- if ( $record_type eq 'p' ) {
248+ if ( $record_type eq 'program' ) {
249 $dbh->do( $program_update_sql, undef, @bind_values, $record_id );
250 }
251- elsif ( $record_type eq 'a' ) {
252+ elsif ( $record_type eq 'agency' ) {
253+ push( @bind_values, $cf{"admin_notes_$counter"} );
254 $dbh->do( $agency_update_sql, undef, @bind_values, $record_id );
255 }
256
257@@ -122,95 +120,109 @@
258 query_name => 'AGENCY_X_EXPIRED_CALL_RECORDS'
259 );
260
261- my %all_notifications = MVHub::Notifications::create_notifications_using(
262- expired_records_aref => $expired_program_records_tocall_aref );
263-
264- %all_notifications = MVHub::Notifications::create_notifications_using(
265- expired_records_aref => $expired_agency_records_tocall_aref,
266- notifications_href => \%all_notifications
267+ my $records_aref = _sort_records(
268+ $self,
269+ $expired_agency_records_tocall_aref,
270+ $expired_program_records_tocall_aref
271 );
272
273- return %all_notifications;
274+ return $records_aref;
275+
276 }
277
278-sub _make_call_records_from {
279- my $self = shift;
280- my %notifications = @_;
281- my $calls_aref = [];
282+sub _sort_records {
283+ my $self = shift;
284+
285+ my $agency_aref = shift;
286+ my $program_aref = shift;
287
288 my $website_name = $self->get_config_param('SITE.website_name');
289
290- foreach my $unique_id ( keys %notifications ) {
291- my $href = $notifications{$unique_id};
292+ my @records = ();
293+
294+ foreach my $agency_href ( sort { $a->{agency_name} cmp $b->{agency_name} }
295+ @$agency_aref )
296+ {
297+ my %call;
298+
299+ $call{contact_last_name} = $$agency_href{contact_last_name};
300+ $call{contact_first_name} = $$agency_href{contact_first_name};
301+ $call{agency_name} = $$agency_href{agency_name};
302+ $call{main_phone} = $$agency_href{main_phone};
303+
304+ $$agency_href{record_type_and_id}
305+ = "$$agency_href{record_type}_$$agency_href{agency_id}";
306+
307+ my @loop_array = ();
308+ push( @loop_array, $agency_href );
309+
310+ my $loop_array_scalar = $$agency_href{record_name} . "\n";
311+
312+ foreach my $program_href (@$program_aref) {
313+
314+ $$program_href{record_type_and_id}
315+ = "$$program_href{record_type}_$$program_href{record_id}";
316+
317+ if ( $$program_href{agency_id} == $$agency_href{agency_id} ) {
318+ push( @loop_array, $program_href );
319+ $loop_array_scalar .= $$program_href{record_name} . "\n";
320+ }
321+
322+ }
323+
324+ $call{CHECKBOX_LOOP_TAG} = \@loop_array;
325
326 my $quick_login_url = MVHub::Utils::generate_quick_login_url(
327 $website_name,
328- $notifications{$unique_id}{agency_id},
329- $notifications{$unique_id}{quick_login_passwd}
330+ $$agency_href{agency_id},
331+ $$agency_href{quick_login_passwd}
332 );
333-
334- delete @$href{
335- qw/public_email reminders_sent last_updated quick_login_passwd agency_contact_email/
336- };
337-
338- $$href{mailto} = _make_mailto_link( $href, $quick_login_url );
339-
340- delete $$href{expired_records_scalar};
341-
342- $$href{CHECKBOX_LOOP_TAG} = _get_checkbox_loop_tag(
343- $notifications{$unique_id}{expired_record_list} );
344-
345- delete $$href{expired_record_list};
346-
347- push @$calls_aref, $href;
348-
349- }
350-
351- # Even if a set of records has different contacts
352- # we want records for an agency grouped together
353- # especially if contact email changes
354- my @ray = sort { $a->{agency_name} cmp $b->{agency_name} } @$calls_aref;
355-
356- $calls_aref = \@ray;
357- return $calls_aref;
358-
359-}
360-
361-sub _get_checkbox_loop_tag {
362- my $programs_aref = shift;
363- my @checkboxes;
364-
365- foreach my $p_href (@$programs_aref) {
366- my %checkbox;
367- $checkbox{checkbox_name}
368- = "$$p_href{record_id}_$$p_href{record_type}_checkbox";
369- $checkbox{record_name} = $$p_href{record_name};
370- $checkbox{last_updated} = $$p_href{last_updated};
371- $checkbox{reminders_sent} = $$p_href{reminders_sent};
372- push @checkboxes, \%checkbox;
373- }
374-
375- @checkboxes = sort _get_checkbox_loop_tag_sort @checkboxes;
376- return \@checkboxes;
377-}
378-
379-sub _get_checkbox_loop_tag_sort {
380-
381- # always sort names containing '(Main Agency record)' first
382- return -1 if $a->{record_name} =~ m/Main Agency Record/;
383- return 1 if $b->{record_name} =~ m/Main Agency Record/;
384-
385- return $a->{record_name} cmp $b->{record_name};
386+ $call{mailto} = _make_mailto_link( $agency_href, $quick_login_url,
387+ $loop_array_scalar );
388+
389+ push( @records, \%call );
390+ }
391+
392+ _delete_keys( $agency_aref, $program_aref );
393+
394+ return \@records;
395+
396+}
397+
398+sub _delete_keys {
399+
400+ my $agency_aref = shift;
401+ my $program_aref = shift;
402+ my @columns_to_be_deleted = qw /public_email
403+ quick_login_passwd
404+ agency_contact_email
405+ agency_id
406+ agency_name
407+ email
408+ contact_fax
409+ main_phone
410+ record_id
411+ record_type /;
412+
413+ foreach my $href ( @$agency_aref, @$program_aref ) {
414+ delete @$href{@columns_to_be_deleted};
415+ }
416+
417+ foreach my $href (@$program_aref) {
418+ delete @$href{"admin_notes"};
419+ }
420 }
421
422 sub _make_mailto_link {
423- Params::Validate::validate_pos( @_, 1, 1 );
424- my ( $record_href, $quick_login_url ) = @_;
425+
426+ Params::Validate::validate_pos( @_, 1, 1, 1 );
427+ my $record_href = shift;
428+ my $quick_login_url = shift;
429+ my $expired_records_scalar = shift;
430+
431 $$record_href{contact_first_name} or croak "contact_first not defined";
432- $$record_href{expired_records_scalar}
433- or croak "expired_records_scalar not defined";
434- $$record_href{agency_name} or croak "agency_name not defined";
435- $$record_href{contact_email} or croak "contact_email not defined";
436+ $$record_href{agency_name} or croak "agency_name not defined";
437+ $$record_href{contact_email} or croak "contact_email not defined";
438
439 my $email_addr = $$record_href{contact_email};
440 my $subject = "Conversation re $$record_href{agency_name}";
441@@ -220,7 +232,7 @@
442
443 It was good to talk to you about $$record_href{agency_name} and:
444
445- $$record_href{expired_records_scalar}
446+ $expired_records_scalar
447
448 When you have time, if you could click the link below:
449

Subscribers

People subscribed via source and target branches