Here's the second patch that improves the UI. This:

  • Changes the status "pending response" to "queued" as requested by Gatsby/Killes on IRC
  • Adds a new status "disabled"
  • Paginates the list of cvs accounts on the accounts overview page
  • Adds a "filter" to the overview page to filter the display on status
CommentFileSizeAuthor
#11 cvs.module_r1.106.2.19.2.28_p27.46 KBAjK
#10 cvs.module_r1.106.2.19.2.28_p17.35 KBAjK
#3 cvs.module_r1.106.2.19.2.26_p27.64 KBAjK
#2 cvs.module_r1.106.2.19.2.26_p17.63 KBAjK
cvs.module_r1.106.2.23_p18.29 KBAjK

Comments

AjK’s picture

AjK commented
Version: 4.7.x-1.0 » 4.7.x-2.0
Assigned: Unassigned » AjK
Status: Needs review » Needs work

As dww pointed out in my other patch, this is against the wrong branch. I will reroll the patch later and then mark it for review.

AjK’s picture

AjK commented
Status: Needs work » Needs review
StatusFileSize
new cvs.module_r1.106.2.19.2.26_p17.63 KB

OK, here's the patch re-rolled for DRUPAL-4-7--2 (the branch d.o uses)

AjK’s picture

AjK commented
StatusFileSize
new cvs.module_r1.106.2.19.2.26_p27.64 KB

*sigh* missed t() on the status strings

dww’s picture

dww
we/he/they
commented
Status: Needs review » Needs work

- syntax error on line 2225 ;)
- code style needs work (bad indentation in cvs_get_status())
- a comment at the top of the define()'s for the status values to remind people to update cvs_get_status() would be nice

dww’s picture

dww
we/he/they
commented
Status: Needs work » Fixed

faster for me to just fix this myself than go through another patch/review. ;)
commited to DRUPAL-4-7--2 as revision 1.106.2.19.2.27 and installed on d.o.
nice UI improvements, thanks!
-derek

Zen’s picture

Zen commented
Status: Fixed » Needs work

Following a visual browse of the committed patch:

  • Why use 'account status' in just the one place and leave the rest as status?
  • #type = textfield.. textfield is not quoted.
  • cvs_get_status is poorly named. cvs_get_status_options / array or something is more appropriate. There is a lot of confusion as to the plural form of status.. but we will do well to not go there at all.
  • Comments not capitalised; missing periods. Same applies to one of the #descriptions.
  • No blank lines after function definition please.
  • If 'all' is being displayed, it doesn't appear to be translated.
  • I don't understand why the #action is required in cvs_account and why it and the #submit are not part of the array block above it. This block of code looks overcomplicated at first sight.
  • SQL query: multiline + concatenated = does not conform to established code style.
  • SQL query: The $result statement below the query adds the session variable as a parameter for the %d in the WHERE which doesn't always exist.
  • Preexisting issue: The cvs_account method uses a print (theme('page...) rather than a return.
  • The cvs_view_filter_submit function follows a dated format - using a global etc. Or it might be some new fangled 5.0 thing. I don't know .. but it's incorrect for 4.7.
  • Typo in the cvs_get_status() docs.
  • In the same function, is the isset check for 'all' necessary?

Thanks,
-K

Zen’s picture

Zen commented

It would also be nice to wrap the filter in a container-inline style.

um, and please ignore my comment on "why are they not in the array block above #action".. I was on drugs at the time.

-K

dww’s picture

dww
we/he/they
commented

thanks, zen. ;) that's what i get for reviewing patches at 4:30am. ;)

i noticed the SQL formatting, but honestly:
a) this isn't core
b) i find a little whitespace goes a long way in helping readability.

so, that's why i let is slide. i'm ok with doing it either way, personally.

otherwise, yeah, everything Zen addressed should be fixed. sorry for being hasty. please roll a new patch against the end of DRUPAL-4-7--2 (since i already committed), instead of re-rolling the patch from #3.

thanks!
-derek

AjK’s picture

AjK commented

I'm away on business at the moment so I'll do this at the weekend

AjK’s picture

AjK commented
Status: Needs work » Needs review
StatusFileSize
new cvs.module_r1.106.2.19.2.28_p17.35 KB

The attached patch addresses Zen's comments in #6 above. However, it doesn't address them all. Those I had problems with I'll note here, feedback would be welcome:-

I don't understand why the #action is required in cvs_account and why it and the #submit are not part of the array block above it. This block of code looks overcomplicated at first sight.

When adding the filter code I just copied the way watchdog.module did it. It used a #action so I just used it (amended to use the correct url). However, after testing you're correct, a #action is not required. Should it therefore be removed from watchdog.module (er, which is Core and I use Core as a good example to follow?)

Regarding the #submit. You are correct. 4.7 fAPI uses the form_id with the appended _submit as the function name for the standard submit function. However, 4.7 and beyond API allow for additional submit functions to be called using the syntax I used. However, it wasn't required to do it that way so I changed it to use the "normal" method.

Regarding the "multiline SQL". I know it's Drupal standard to do everything on one line. I like the Drupal standards with this being the one exception. If it's simple SQL one line is good. But when the SQL aquires a degree on complexity the additional whitespace often helps in readability (my personal opinion). However, for the sake of Drupal coding standards I have made it one line.

I've also wrapped it in an isset() to ensure that if the session variable isn't set then the pager_query() isn't passed a none existent variable.

Regarding the SQL plus concat. The value being "concat" in this case is $where. This variable itself contains the %d taken for the filter value. If you can tell me how to pass a string variable to pager_query() with an embedded integer %d value (and be sure the preg_match_replace() function used in db_query() spots the %d buried in a %s) then I'm up for changing it and will follow advice on that (as I can't see how to do it, but the dynamic $where is required to apply the filter).

In the same function, is the isset check for 'all' necessary?

Probably not, but it's there to ensure we don't return an "out of scope" value which imho is just as bad as passing an unset var to a function (as in your point regarding the session var possibly being passed to pager_query() when it may not exist). "Belt and braces" sort of thing.

regards,
--AjK

AjK’s picture

AjK commented
StatusFileSize
new cvs.module_r1.106.2.19.2.28_p27.46 KB

I missed/added these points:-

  • Introduced a space that isn't Drupal coding standards, removed the space character
  • Removed the use of the global from the form callback function
  • Missed a full stop of the end of a comment line
Zen’s picture

Zen commented
Status: Needs review » Fixed

Committed along with the following additional fixes:

  • Missing t() around first 'all'.
  • Removed unused $filter_status.
  • Set default filter to a status constant rather than 0. Changed from 'pending' to 'queued'.
  • The ternary $where statement was screwy.
  • Fixed instances of double indentation.
  • Removed $element from theme_pager.. defaults to 0.
  • Added container-inline style to filter form.
  • cvs_list_view_form_submit: $form to $form_values for consistency.
  • Capitalised status options.
  • Fixed insert query in cvs_application_form_submit to default to CVS_QUEUED rather than 0 (CVS_PENDING).
  • Removed $index argument from cvs_get_status_options as it is not used anywhere.

Cheers,
-K

dww’s picture

dww
we/he/they
commented

cool, patch looks good. thanks for the updated patch, ajk, and for the careful reviews, zen.
updated the code on d.o to the latest in DRUPAL-4-7--2 branch.

cheers,
-derek

AjK’s picture

AjK commented

so I nearly got it right ;)

Anonymous’s picture

(not verified) commented
Status: Fixed » Closed (fixed)