Reorganize prod_monitor module [#1419746]

The intent of this issue is to hold information/patches that deal with reorganizing the prod_monitor module. I will be committing patches to review and (hopefully) they are headed in the right direction and we can commit them to the project.

@malc0mn: I liked your comment "Let's make this module the Drupal standard for monitoring your site(s)! (Can I say that out loud? :-D )"...Let's do it! :)

Comment	File	Size	Author
#11	prod_check-new-features.zip	440.53 KB	bocaj
#5	1419746-Reorganize prod_monitor module-5.patch	4.6 KB	bocaj
#4	1419746-Reorganize prod_monitor module-4.patch	4.26 KB	bocaj
#3	1419746-Reorganize prod_monitor module-3.patch	17.2 KB	bocaj
#2	1419746-Reorganize prod_monitor module-2.patch	13.65 KB	bocaj
#1	1419746-Reorganize prod_monitor module-1.patch	11.51 KB	bocaj

Comments

Comment #1

bocaj commented 28 January 2012 at 16:08

Status	File	Size
new	1419746-Reorganize prod_monitor module-1.patch	11.51 KB

This patch is the beginning work to start reorganizing the user interface. I have posted comments in the files themselves to hopefully clarify my intent.

This patch is not commit worthy! This is a proof-of-concept patch!

Comment #2

bocaj commented 28 January 2012 at 16:38

Status	File	Size
new	1419746-Reorganize prod_monitor module-2.patch	13.65 KB

This patch moves the overview page theme function to prod_monitor.theme.inc and moves the functionality out of drupal_get_form in the menu since this just displays information and isn't connected to a form anymore.

Again, this patch is not commit worthy! Proof-of-concept!

Also, this patch builds on top of the patch in #1, it is not a replacement for that patch. For this patch to apply, the patch in #1 has to be applied first. I did this to hopefully help the review process so that we don't have to review one very large patch, but can inspect multiple smaller patches in steps. Hope that works for you! :)

Comment #3

bocaj commented 28 January 2012 at 17:23

Status	File	Size
new	1419746-Reorganize prod_monitor module-3.patch	17.2 KB

This patch moves the site-specific functions add and edit into it's own file (prod_monitor.site.inc) just for better maintainability and organization. Also, this patch begins the functionality of a working add site form and shows the layout of the edit site form (although this form doesn't function properly at all yet!)

Also, this patch reorganizes the code that handles the add/edit forms so that we can display them differently (add site form is broken up into two steps whereas the edit form contains all of the information on one page) even though they have the same fields. I think this layout (the code layout) is easy to understand and should be easy to maintain going forward.

Again, this patch is not commit worthy! Proof-of-concept!

This patch is dependent on the second patch being applied first, which itself is dependent on the first patch. Please make sure that you apply the patches in order for them to apply correctly!

Comment #4

bocaj commented 28 January 2012 at 17:56

Status	File	Size
new	1419746-Reorganize prod_monitor module-4.patch	4.26 KB

Now we're getting somewhere! :)

This patch contains working site add and edit forms! The code is still not optimal, but I think it's heading in the right direction. Fetching the data from the remote site is also functional again! I think this is getting pretty close to being completely functional! Then we'll have to move into code cleanup...assuming that @malc0mn likes the direction that this is heading.

Even though this patch provides working code, it's still not commit worthy!

As before, this patch must be applied in order, apply the first, then second, then third, then this one! :)

Comment #5

bocaj commented 28 January 2012 at 18:33

Status	File	Size
new	1419746-Reorganize prod_monitor module-5.patch	4.6 KB

This patch adds the ability for a user to give the site a name that will display in the overview table and can be used (not implemented yet) to create better page titles that will help with smaller resolution monitors as well as help with status messages.

Also, this patch renames prod-monitor.css to prod_monitor.css (underscore separator instead of dash) to comply with Drupal standards. NOTE: This isn't included in the diff, you will need to manually rename prod-monitor.css to prod_monitor.css for this patch to apply properly!

Still, this patch is not commit worthy!

Please remember to commit patches in order! :)

Comment #6

bocaj commented 28 January 2012 at 18:37

@malc0mn: I have to leave for awhile. I'm excited to hear what you think about the direction these patches move the prod_monitor module. Hopefully they help to (some day) make it part of the "Drupal standard for monitoring sites" (along with prod_check of course)!! :)

Comment #7

malc0mn commented 28 January 2012 at 20:00

Whoa! You spent some time on it!! When I find a moment, probably tomorrow evening, I'll apply them locally and see where you're going...

What I've always wanted to do with the D7 version is convert everything to render arrays. So the helper function to theme to a table definately has to go (have you seen the D7 way? man that's easy...)

Edit: issue is set to 6.x-1.1-alpha2, is that correct?

Comment #8

bocaj commented 29 January 2012 at 05:44

Version:

6.x-1.1-alpha2

» 6.x-1.1

Yeah. Actually everything that I develop in now is D7. The reason I have been doing the development for prod_monitor in D6 is because the site that I use it on is my Aegir site, which is only available on D6. I'd really like to get all of this pushed into D7.

Anyway, hopefully the work I did is heading in the right direction. I think we can definitely use it to build on. I'm curious to see what your thoughts are.

I changed the issue version to 6.x-1.1, I guess that is the latest version? Can you create a dev release so that we can choose that for developments like this? I think it would be nice to commit development releases as well, so we can test on other sites by just downloading the package from Drupal. Plus, I don't think it's a good idea to push new features to a "stable" release (even alpha, beta) all the time. That triggers update notifications for users that have the update module turned on and that can become annoying when they don't need to be updating all the time. Just my two cents.

Comment #9

malc0mn commented 29 January 2012 at 08:40

I agree... This one is definitely in need of dev releases, maybe even should go to a 2.x (will check your work this evening).

mlc

Comment #10

malc0mn commented 29 January 2012 at 13:24

Found some time in between, but the patches don't apply all that well...

For this huge makover patches are not that nice anyway (IMHO), so I made you co-maintainer. Added a 6.x-1.x-dev branch. Could you commit there?

Makes all our lives easier :-D

Comment #11

bocaj commented 29 January 2012 at 15:38

Status	File	Size
new	prod_check-new-features.zip	440.53 KB

I was afraid of the patches having issues. I am having issues committing for some reason (I'll check into that later when I have a bit more time) but I zipped up the project including all the changes I made that the patches include. Hopefully this will at least let you be able to review it.

Comment #12

malc0mn commented 29 January 2012 at 20:18

There was indeed an issue with that branch. Fixed it and committed the contents of the zip file...
(Fixed small bug in the hook_update_N() you added along the way.)

The new setup is indeed better. Just wondering: when adding a site I first checked to see if we could connect to it. You have omitted that step on purpose or it's not 'transfered' yet? It still does throw the error, but allows you to save the site anyway...

Might be a good thing, might not. Not sure yet. It does allow you to add a bunch of sites and configure them properly later on, but that just seems like a silly workflow :-D

Edit: the protocol as a dropdown is OK, but I would not make it a separate field in the database. That really seems silly to me. If we ever want to filter on it, we can simply

SELECT * FROM {prod_monitor_sites} WHERE url LIKE 'https://%'

Comment #13

bocaj commented 30 January 2012 at 00:48

There was indeed an issue with that branch. Fixed it and committed the contents of the zip file...
(Fixed small bug in the hook_update_N() you added along the way.)

Thanks for committing the zip file for me and for fixing the bug that I had in the hook_update_N().

"Just wondering: when adding a site I first checked to see if we could connect to it. You have omitted that step on purpose or it's not 'transfered' yet?"

Yeah, I think it would definitely be better to throw the error and not proceed until it can actually connect properly. That would be a lot more useful I think. My main goal in these patches was to get a workflow ironed out, not concentrating as much on full functionality (hence why I kept warning that these aren't commit worthy. Someone not testing, but actually trying to use the module could get kind of frustrated :).

the protocol as a dropdown is OK, but I would not make it a separate field in the database. That really seems silly to me. If we ever want to filter on it, we can simply...

I personally think the protocol as a dropdown is much better for usability. Plus, we don't have to run any validation checks to make sure that the user entered one and they don't have to remember themselves to enter it. I'm not sure what I think about not having the protocol in it's own field. I guess it doesn't really matter. I personally think that it makes the most sense to have a few more fields and things split apart more than having fewer fields and cramming things in together. I personally think it would be better to have separate fields for the API key, the functions and the checks (none of those are included in the patches) instead of one large settings field. Therefore, I tend to think that having the protocol in a separate field as well would make more sense.

Having said all that, you are the official maintainer of the project, so you get the final say! :)

Comment #14

bocaj commented 30 January 2012 at 02:59

Branch is now fixed, I was able to commit...thanks!

Committed small update that changes the overview table so that a simple message is displayed if there are no sites configured to be monitored instead of a blank table.

Comment #15

malc0mn commented 30 January 2012 at 19:03

I'm not sure what I think about not having the protocol in it's own field. I guess it doesn't really matter.

There is a special function to handle urls in PHP being parse_url(). With this you can easily split protocol from the rest in the edit form instead of using regex.

Settings vs. checks etc could indeed be separate. We'll have to see the benefit of it query wise. Never wrong to give it a shot!

Comment #16

bocaj commented 30 January 2012 at 20:10

Interesting...I didn't know about the parse_url() function. It's always nice learning something new! Thanks! :)

Having such an easy function call like that to make I can't see any useful reason to separate out the protocol from the url.

I'll keep working on finalizing stuff in the dev branch. I think it's coming along nicely! I've been trying to remember to commit regularly so that there isn't large diffs to look through. I'm not sure if you're developing on the dev branch too or not, but hopefully that's not messing you up if you are.

Comment #17

malc0mn commented 30 January 2012 at 20:13

just committed a small bugfix to the dev branch for prod_check & module update checking.
If you merge properly, nothing will break. Git's quite good at that.

Working on prod_check 7.x to finally get the remote update status checking ported. Wanted that in the 1.x version as well to align it with D6...

Comment #18

bocaj commented 30 January 2012 at 22:09

I'm looking at the module status/update functionality. Is this not implemented yet or what am I missing? I can't seem to figure out how this works. All the settings I try both on the site running prod_check and the site running prod_monitor don't seem to change anything. The prod_monitor_site_modules table is always empty. Can you help me figure out what I'm missing? Thanks!

Comment #19

malc0mn commented 30 January 2012 at 22:15

Aha, the pitfall :-D

Culprit is this one:

<?php
function _prod_check_module_list($caller = 'internal') {
  [...]

  // The if() is split up this way for full perfomance: we only run once a week,
  // so on 6 out of 7 days, we won't pass the first if statement.

  // First check if we are scheduled to run this day of the week.
  if (variable_get('prod_check_module_list_day', 0) == date('w', $now)){
    // First check if we already ran today.
    if (date('Ymd', $last) != date('Ymd', $now)) {
?>

Comment #20

malc0mn commented 30 January 2012 at 22:46

Got it working?

Finished the remote update checking for D7 as well. Completely core independant ofcourse :-D

Now D7 and D6 are completely aligned (safe for the user password check, don't think that is doable on D7 without taking down the site with an insane while loop or running a cron which is not worth it IMHO).

Comment #21

bocaj commented 30 January 2012 at 22:47

Thanks!

OK...so it looks like the reason I was having issues is because of the issue you just fixed in the D7 branch (http://drupalcode.org/project/prod_check.git/commit/62a229d)!! The system_rebuild_module_data and system_rebuild_theme_data functions weren't named correctly! Thanks for updating that. Now I see the module update stuff! VERY cool!!

Side note: Do you happen to use Skype? If so, we can IM instead of posting in this issue. I don't mind posting in this issue, just thought I'd ask. If you do use Skype, send me a private message.

I think this module is coming together really well! :)

Comment #22

bocaj commented 30 January 2012 at 23:04

I spoke too soon. Once I actually updated to the newest commit then it didn't work anymore. Previously I just changed the two function names mentioned above. Then I updated to the new commit and it is throwing an error again.

I'm using prod_monitor on a D6 site to check a D7 site, if that helps! :)

Comment #23

malc0mn commented 31 January 2012 at 10:29

Blast... That won't work indeed... Other way around should work...

I'll probably have to adjust prod_mon for D6 to handle the slightly different data structure coming from a D7 site...

The update status checking never worked in the D7 prod_check version. I've made it work yesterday for D7 but did not think of using D6 to monitor a D7 site. G#dd!mn...

(I don't have skype ;-) )

Edit: what error do you get? I'll fix this evening...

Comment #24

bocaj commented 31 January 2012 at 16:36

Weird...so I tried it again this morning to get the exact error for you (it was just the standard, can't get modules from remote site error created by prod_monitor) but it seems to be working just fine this morning. I'll continue testing and let you know what my results are!

Comment #25

bocaj commented 31 January 2012 at 16:43

OK. So it's not working. I'm not sure why it did when I first tested.

Here's what's going on...when I add a site (D7) to be monitored (with my D6 site running prod_monitor) everything works fine as far as the checks from prod_check. When I go to admin/reports/prod-monitor/site/%/view/updates I get the standard text saying that no information is available. I click the "Check manually" link and then I get an error message saying: "No module data available: cannot check for updates!"

There are no records for the remote site in prod_monitor_site_modules table. I will try to test out a D6 to D6 site a little later on. Also, haven't tried a remote D6 site on a D7 site running prod_monitor. Will try to test that out as well.

Comment #26

malc0mn commented 31 January 2012 at 17:49

bocaj, can't see anything wrong, works as designed... When I saw your message this morning I thought I forgot something with D7 and the updates, but I covered it allright...

You need to know that the module data fetching from the remote site is very very strict within time... You can only fetch it once a week within a certain timeframe (there are some open issues concerning this strict timeframe & refetching of the data).

The reason for this strictness is that the module data can be really huge, depending on how many modules your site has... As I reported before, here is the check:

<?php
function _prod_check_module_list($caller = 'internal') {
  global $base_url;

  $check = array();
  $now = REQUEST_TIME;
  $last = variable_get('prod_check_module_list_lastrun', 0);

  // The if() is split up this way for full perfomance: we only run once a week,
  // so on 6 out of 7 days, we won't pass the first if statement.

  // First check if we are scheduled to run this day of the week.
  if (variable_get('prod_check_module_list_day', 0) == date('w', $now)){
    // First check if we already ran today.
    if (date('Ymd', $last) != date('Ymd', $now)) {
      $time = explode(':', variable_get('prod_check_module_list_time', '03:00'));
      // Only run if we are spot on, or past the scheduled point. This CAN cause
      // a run hours after the scheduled time, all depending on the cron setup
      // on the prod_monitor site!
      if (date('H', $now) >= $time[0] && date('i', $now) >= $time[1]) {
?>

just remove the IFs for now or make them all if (TRUE) { for easy testing...

Comment #27

bocaj commented 31 January 2012 at 18:17

Hmmm...still no go for me. I got the initial update check to work, but then after that it hasn't been working. The update status page in prod_monitor doesn't match the update status page on the D7 site. I'm assuming though that I'm doing something wrong so I won't worry about it at this stage.

Comment #28

malc0mn commented 26 March 2013 at 19:59

moved work on this to 6.x-1.x-1419746 branch

Comment #28.0

malc0mn commented 26 March 2013 at 19:59

Issue summary:

View changes

Updated issue summary.

Comment #29

anybody

German

Porta Westfalica

commented 2 May 2023 at 09:58

Issue summary:	View changes
Status:	Active	» Closed (outdated)

D6 is dead dead.

Reorganize prod_monitor module