330487 - Automatic Update Notification for Bugzilla

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, enhancement)

Description

[Max Kanat-Alexander]

This was brought up in our IRC meeting today. It would be nice to have an XML file on bugzilla.org that Bugzilla could check, and inform the administrator when he's logged in that there's a new version.

We were thinking of having it just be displayed on editparams.cgi, but it could also be somehow displayed in the footer.

I think it should notify them of the latest stable release, and the latest release on their branch.

We should also consider the load on bugzilla.org, and not make Bugzilla download the file too often.

Comment 1

[Max Kanat-Alexander]

We could also have the option (disabled by default) to send information to bugzilla.org about what version is being run, and then we'd have some statistics about versions of Bugzilla in use, out in the field, which we've always wanted to have.

Comment 2

[Frédéric Buclin]

(In reply to comment #1)
> We could also have the option (disabled by default) to send information to
> bugzilla.org about what version is being run

That's definitely another bug. I will put my "veto" on it (in this bug, that is).

Comment 3

[Colin Ogilvie [:cso]]

Attachment: Possible XML file

I was thinking about the format of the XML file to send from Bugzilla.org and came up with this simple one.

Would it not be possible to make Bugzilla request a url like "http://www.bugzilla.org/versioncheck.xml?version=BZ_VERSION" which would achieve both, and a perl script could just send the required version information, rather than all possible updates?

Comment 4

[Jacob Steenhagen]

My opinion is that this should not be in the Generic footer. I mean, would it do me any good to be alerted that there's a newer version out than what runs here on BMO? Not really... there's nothing I can do about it. I think this should only be displayed in administrative pages.

Comment 5

[Colin Ogilvie [:cso]]

(In reply to comment #4)
> My opinion is that this should not be in the Generic footer. I mean, would it
> do me any good to be alerted that there's a newer version out than what runs
> here on BMO? Not really... there's nothing I can do about it. I think this
> should only be displayed in administrative pages.

It could be in the generic footer for Administrative users - who can see the admin pages anyway...

Comment 6

[Jacob Steenhagen]

To me at least, that would be fine... as long as "ordinary" users aren't seeing it.

Comment 7

[Frédéric Buclin]

Comment on attachment 216413 [details]
Possible XML file

><bugzilla>
>    <version vID="2.18.4">
>        <update vID="2.18.5" url="http://www.bugzilla.org/releases/2.18.5/" />
>    </version>
>    <version vID="2.16.10">
>        <update vID="2.16.11" url="http://www.bugzilla.org/releases/2.16.11/" />
>    </version>
></bugzilla>


I was thinking about something like:

<bugzilla>
  <branch id="2.16">
      <version>2.16.11</version>
      <status>closed</status> (I'm anticipating the release of 2.22 :))
      <url>http://www.bugzilla.org/releases/2.16.11/</url>
  </branch>
  <branch id="2.18">
      <version>2.18.5</version>
      <status>stable</status>
      <url>http://www.bugzilla.org/releases/2.18.5/</url>
  </branch>
  ... (same for 2.20 and 2.22)
  <branch id="tip">
      <version>2.23.1</version>
      <status>development</status>
      <url>http://www.bugzilla.org/releases/2.23.1/</url>
  </branch>
</bugzilla>

I think a "status" field would be useful as we could then offer the following options to the user:

 Choose one of the following options:
1) latest development snapshot (e.g. 2.23.1)
2) latest stable release (e.g. 2.22)
3) latest stable release of the branch (e.g. 2.18.5 assuming the user has installed version 2.18.x)
4) do not check for updates

Comments?

Comment 8

[Colin Ogilvie [:cso]]

(In reply to comment #7)
> <bugzilla>
>   <branch id="2.16">
>       <version>2.16.11</version>
>       <status>closed</status> (I'm anticipating the release of 2.22 :))
>       <url>http://www.bugzilla.org/releases/2.16.11/</url>
>   </branch>

Status should probably be an attribute of the branch, rather than a seperate element.

> I think a "status" field would be useful as we could then offer the following
> options to the user:
> 
>  Choose one of the following options:
> 1) latest development snapshot (e.g. 2.23.1)
> 2) latest stable release (e.g. 2.22)
> 3) latest stable release of the branch (e.g. 2.18.5 assuming the user has
> installed version 2.18.x)
> 4) do not check for updates
> 
> Comments?

Sounds better than what I came up with.

Comment 9

[Frédéric Buclin]

Attachment: patch, v1

Here is a first shoot. The patch works, using the XML file actually located at:
http://landfill.bugzilla.org/csodocs/bugzilla-update.xml

The goal of this patch is mainly to get something to discuss about. All comments and suggestions are welcome.

Comment 10

[Frédéric Buclin]

I have set up http://landfill.bugzilla.org/notification/ with this patch applied. I have also hacked $Bugzilla::Config::VERSION to decrease the version from 2.23 to 2.18.2 to simulate an old installation. This way, anyone with admin privs can test the different options.

'development' will point to 2.23.1
'stable' will point to 2.20.2
'stable for the current branch' will point to 2.18.5 (because that's the latest version available for the 2.18 branch)

Go have a look and give me some feedback.

Comment 11

[Vlad Dascalu]

I kind of maintain a public Bugzilla installation. I just checked: it's a 2.19.1+ installation, and I haven't checked upon it in ages.

A notifier in the footer is not going to make me login to see it, because I wouldn't have known about such a thing.

I guess in specific cases like this one, the only thing that would do the trick would be an email notification.

Design-wise, we would retain somewhere the latest BZ version fetched by LWP, and when we get something different, we send the email and update the value retained.

Just some brainstorming, this can be left in another bug if it doesn't make it.

Comment 12

[Vlad Dascalu]

Oh, I thought about subscribing to announce@, but not all BZ admins do that (some don't even know about it), and having an UI checkbox regarding update notifications would be probably easier for them.

Comment 13

[:glob ✱]

Comment on attachment 217960 [details] [diff] [review]
patch, v1

> # Return the appropriate HTTP response headers.
> print $cgi->header();
> 
>+# Look for new releases.
>+my $new_release;
>+if ((Param('upgrade_notification') ne 'do not check for updates')
>+    && $user->in_group('admin'))
>+{

isn't this a big hit to do for every page view, even if it is admin-only?

while mirror() should only do a HEAD request, that's a lot of unnecessary hits against the server hosting the xml.

i think it would be better to limit the checking to once per day (or week?) and persist $new_release to ensure the admin is always alerted to updated.

>+    require LWP::UserAgent;
>+    require XML::Twig;

these should be in checkparams.

>+        my $twig = XML::Twig->new();
>+        $twig->parsefile($xml_file);

parsefile() dies if the xml is invalid; it would be better to use safe_parsefile() and do nothing if it fails.

Comment 14

[Frédéric Buclin]

Attachment: patch, v1.1

This patch correctly manages release candidates using the following and arbitrary conversion about the subversion:

rc1 => -9 (e.g. 2.22rc1)
rc2 => -8
...
rc9 => -1
".0" => 0 (i.e. the final release, e.g. 2.20, 2.22)
.1  => .1 (e.g. 2.22.1)
.2  => .2
...

This way, 2.22rc1 is converted to version = 2.22 and subversion = -9, so that it will be seen as older than 2.22 final (version = 2.22 but subversion = 0).

A user who installed a RC must choose the 'latest stable release of the branch' option in order to get notifications for new RCs.

I finally don't want to display the notification in the footer anymore as it could irritate administrators to see it on *each* page. Having it on index.cgi is enough IMO.

Comment 15

[Frédéric Buclin]

(In reply to comment #13)
> isn't this a big hit to do for every page view, even if it is admin-only?

It only affects index.cgi, not the other pages. But yes, probably we need to check only once per day or even per week.


> persist $new_release to ensure the admin is always alerted to updated.

How do you do that? Either we assume we don't want to parse the local XML file everytime and we have to store $new_release somewhere (where?) or we assume parsing the XML file everytime is fine and we only have to check for a newer version of the XML file once per X days.

Comment 16

[:glob ✱]

> > persist $new_release to ensure the admin is always alerted to updated.
> 
> How do you do that? Either we assume we don't want to parse the local XML file
> everytime and we have to store $new_release somewhere (where?) or we assume
> parsing the XML file everytime is fine and we only have to check for a newer
> version of the XML file once per X days.

parsing the xml file each run should be ok, but my feeling is that if we can avoid loading the XML libraries we should.

maybe write the $new_release keys to a delimited file in a specific order so we can quickly read them back in again?

Comment 17

[Frédéric Buclin]

Attachment: patch, v2

Complete patch, fixing all comments reported by glob.

Comment 18

[Frédéric Buclin]

Attachment: patch, v2.1

Write a better description of the parameter in editparams.cgi.

Comment 19

[Frédéric Buclin]

Note to self:

when committing the patch (or updating it), use:

updates.bugzilla.org/bugzilla-update.xml for the permanent URL (per discussion with justdave during the IRC meeting today).

Comment 20

[Frédéric Buclin]

Attachment: patch, v2.2

2 fixes:
- Update the permanent URL to 'http://updates.bugzilla.org/bugzilla-update.xml';
- The number of seconds in a week is 604800, not 25200. :)

Comment 21

[Frédéric Buclin]

Attachment: patch, v2.3

Add a notice "This message is only shown to logged in users with admin privs." on index.cgi, as requested by justdave.

Comment 22

[Frédéric Buclin]

Attachment: patch, v2.4

justdave said the notice was too small. Increasing the text size from 50% to 70%.

Comment 23

[Frédéric Buclin]

Comment on attachment 218082 [details] [diff] [review]
patch, v2.4

hey vladd, I hope you're faster than all these lazy reviewers... :-p

Comment 24

[Jacob Steenhagen]

Note that I've only looked at the code, I have not tested this, but it looks like there's a couple of errors in handling version numbers.

1. The branch is always treated as "major.minor" and a numeric comparision is made to see if the branch_ver is larger than the current_branch. Where we're at now, this is unlikely to cause problems, but as we move into 3.x, it can. FE, numerically speaking, 3.2 is larger than 3.12, 3.12 will be a newer version.

2. There appears to be a similar error when dealing with the sub_version. According to the way I'm reading it, the following table shows some version numbers and what the sub_version variable will show for them:

version      intermediate var       final sub_version
2.24rc2           rc2                       -8
2.24.5            .5                        .5
2.24.11           .11                       .11

In the numeric eval, .11 is smaller than .5 even though it's the newer release. This problem only happens if we have more than 9 subversion releases on a branch. Not likely, but possible.

Again, I've only read the code, I haven't tested it, so I'm not setting a review- as my reading could be entirely wrong.

Comment 25

[Frédéric Buclin]

Attachment: patch, v3

Despite the patch says it points to http://updates.bugzilla.org/bugzilla-update.xml, you have to temporarily use http://updates.bugzilla.org/bugzilla-update2.xml in order to test my patch, till justdave either updates it or gives me write access to this file.

Comment 26

[Max Kanat-Alexander]

(In reply to comment #25)
> till justdave either updates it or gives me write access to this file.

  You have write access to the file, now.

Comment 27

[Colin Ogilvie [:cso]]

Comment on attachment 220202 [details] [diff] [review]
patch, v3

>Index: template/en/default/index.html.tmpl
>+[% IF release %]
>+  <div id="new_release">
>+    A new release is available: [% release.latest_ver FILTER html %]<br>
>+    This release is available at
>+    <a href="[% release.url FILTER html %]">[% release.url FILTER html %]</a>.<br>
>+    Release date: [% release.date FILTER html %]<p>
>+
>+    <p class="notice">This message is only shown to logged in users with admin privs.</p>
>+  </div>
>+[% END %]
>+

Should the URL be FILTER uri rather than html - or even both?

Comment 28

[victory <[e-mail]>]

Attachment: v3-make better html

Comment 29

[Vlad Dascalu]

Comment on attachment 222891 [details] [diff] [review]
v3-make better html

Moving review? flags on the latest attachment (after reviewing the diff between them --it looks cool, thanks RSZ!)

Comment 30

[Vlad Dascalu]

Comment on attachment 222891 [details] [diff] [review]
v3-make better html

Ok, so the reason for review- is actually the fact that I don't like how it bails out when it can't read the update file.

For example, if notifications are turned on, but we can't read that file (maybe a permission thing), we should display to the admins "Currently update notifications are turned on, but Bugzilla can't read the xxx file, and therefore we are unable to check the latest version. Please do this to correct the issue, and then reload this page."

Same for the case where we're unable to write to the updates file. I don't remember the times I've did "chmod 755 -R bugzilla" due to crappy checksetup.pl permissions, and I'd hate to disable the updates due to this.


Now, as nits:

The names that you selected as param values are the longest English-text param-values for drop-down selection in the param thing. The nearest is sslbase, and that is 2 words at most. Localization would prove increasingly difficult with that. 

See bug 252739 for a related example (yeah, I know template choices aren't translatable).

One idea that I reluctently like:

We could use numbers, and explain in /admin/params/core.html.tmpl what each number does. Eww.

Actually numbers could be intuitive, the lower the number, the more stable you want to be:
- 0 would be disabled
- 1 would be same-branch
- 2 would be latest stable
- 3 would be development.

And we could explain it as "channel selection" in that  /admin/params/core.html.tmpl thing.And then Germans, Japanese and Romanians could translate the template and everything would make sense for everybody.

Another nit:  what should happen if I select the latest release for the current branch, and my branch is, let's say, 2.24? And 2.24 is closed in the future? Shouldn't we have some way of indicating in bugzilla-update.xml that a branch was closed?

This could go in another bug, but  if you land this bug on the trunk, and don't find active reviewers for such an enhancement later, then 2.24 would go in freeze, you would be unable to add the feature to the 2.24 train, and 2.24 folks will stop getting notifications when 2.24 will close.

In chat I said:

vladd-updateNotifyReview besides, all the logic is in a few lines:
vladd-updateNotifyReview +        # We do a string comparison instead of a numerical one, because
vladd-updateNotifyReview +        # e.g. 2.2 == 2.20, but 2.2 ne 2.20 (and 2.2 is indeed much older).
vladd-updateNotifyReview +        @release = grep {$_->{'branch_ver'} eq $branch_version} @releases;
vladd-updateNotifyReview +    }
vladd-updateNotifyReview +
vladd-updateNotifyReview +    # Return if no new release is available.
vladd-updateNotifyReview +    return 0 unless scalar(@release);
vladd-updateNotifyReview Basically you need to add an if after that grep, and to match a more recent branch if none got returned.


Also, I don't like this "else":

+    }
+    else {
+        # We want the latest stable version for the current branch.

We should elsif match the last option as well, and then return an error ("invalid param value") if it's not that either. Just to be on the safe side of things. But this is certainly a nit as well.

Comment 31

[Vlad Dascalu]

Comment on attachment 222891 [details] [diff] [review]
v3-make better html

+        eval("require LWP::UserAgent");

We should probably add LWP::UserAgent to the optional modules list thing in checksetup.pl.

Comment 32

[Vlad Dascalu]

I should probably shut up now :), but I'm adding this for reference:

If there is some kind of glitch, and we get a timeout on the LWP thing, we still update the file's timestamp.

So, if we release today let's say 3.0, the system could theoretically check the version 7 days from now, get a timeout, and then wait another 7 days until the next fetch.

The simple alternative would be to simply not update the timestamp if we encountered a timeout or another error. But this would be even more "destructive": the update.bugzilla.org website could be down for 2-3 days, and then we would have a bunch of servers, trying to fetch the URL on every index.cgi hit.

The right fix is to properly have 2 intervals (I'm actually inspired here by the DNS TTL specification): the first is the regular 7 day thing, and the second is used in case of a negative response. It could basically say: refetch the page after 86400 seconds (1 day) if you encountered an error.

But I'll leave that in another bug, I'd settle in this one with channel numbers as param options for example :)

Comment 33

[Frédéric Buclin]

Attachment: patch, v4

Fix most vladd's comments.

Comment 34

[Vlad Dascalu]

Comment on attachment 224950 [details] [diff] [review]
patch, v4

Woo, looks like we'll get this in after all this week or so :) This is really a cool feature! Seriously!

Here are my review notes, sorry for not catching some of this stuff earlier:

-> the entire update thing should be in Bugzilla::Update, we really don't want to pollute Bugzilla::Config with this. Furthermore, maybe in the future, guys that don't want notifications would simply delete Bugzilla::Update (or maybe in the future we can make this a plugin, or even a CPAN module). The code reuse opportunities, the number of perl webapps that could use this code, and so on, are too great in order to hide this in Bugzilla::Config (and pollute it). Also, only index.cgi should use Bugzilla::Update for now, which doesn't slow down the other web pages when they use Bugzilla::Config (their Perl code that needs to be parsed remains the same).

-> Bugzilla->user is really not suitable to be accessed from Bugzilla::Update; the Bugzilla->user->in_group('admin') check should be moved in index.cgi (if in_group() { $vars->{'release'} = ... }). We should be able to have let's say a cron job that simply uses Bugzilla::Update and checks nightly for updates, without the authentication hassle.

-> the patch should add 'LWP-UserAgent' to the PPM hash located at the beginning of checksetup.pl (that's the exact name of the ppm --I'm saying this since I know you don't have a Windows installation to get this information).

-> error messages should specify explicitly the local XML file path, otherwise the admin can't make sure that it has correct rights.

-> "'stable_branch_release' notifies you only about new releases corresponding to the branch your installation is based on": well that's no longer 100% true now (the exception case is when the branch is closed)

-> the eval("require XML::Twig") should be moved when it's needed, right before the XML::Twig->new() thing.

-> get_notifications in Bugzilla::Update should invoke two procedures:
     syncronize_file('http://updates.bugzilla.org/bugzilla-update.xml', "$datadir/bugzilla-update.xml");
     get_notification_hash("$datadir/bugzilla-update.xml", $Bugzilla::Config::VERSION, Param('upgrade_notification'));

     get_notifications should call those two functions above; I really want those 2 to be stateless, that is to receive all the params that they need upon calling them. I don't care about having get_notifications stateless, so it can still have zero params.

Comment 35

[Frédéric Buclin]

Attachment: patch, v5

I fixed all but the last 3 comments vladd made in his previous review, for the following reasons:
- eval("require XML::Twig"); is called very early to avoid downloading the XML file if we are unable to parse it anyway. There is a comment already explaining this;
- I don't want to add another comment in core.html.tmpl to explain what happens when a branch is closed. Descriptions must remain small. This kind of edge-case should be described in docs only;
- I see no reason to have a separate synchronize_xml() and get_hash(). This makes the code more complicated as get_notification() would have to check the returned value to know what to do with it (success/failure). This could be done in a separate bug if you can show me a testcase where this would be useful to have.

Comment 36

[Vlad Dascalu]

Comment on attachment 224992 [details] [diff] [review]
patch, v5

I failed to get an update notification for the following scenario:

I modified the XML to point out 2.24.2 as the current stable release. I've selected the "stable_branch_release" option, and then I've tested it with the current version "2.23.1+". I didn't manage to get the notification for 2.24.

According to the documentation, 'stable_branch_release' notifies you only about new releases corresponding to the branch your installation is based on. So for all those running devel versions, and wanting to get notifications about stables, it doesn't work.

I suspect the reason is the fact that $branch_version is "2.23", and then when it greps for that it doesn't find the 2.24 information. We should special-case numbers that are not divisible with 2 :)

Two more issues:

-> the red box needs to say something in the lines of "This is an admin-only notice". I kind of imagined Mitchell coming to b.m.o., seeing the box due to her admin privs, and then screaming: "justdave, what's up with this red stuff on top of the b.m.o. homepage?" :-)

-> we need to establish what to do if we're running 2.23.1, 2.23.2 is available, and "stable_branch_release" is selected. Or what to do when 2.24-rc1 is released (the rest of the scenario is the same).

Comment 37

[Vlad Dascalu]

Just to elaborate/be more clear about my first scenario from the previous comment:

I'm running 2.23.1 (because it has some very cool feature, very important for me, that 2.22 lacks). I don't want to go forever with devel, in fact I want to be conservative and keep the safest path: go down the 2.24 route. I don't even want 2.26/3.0 when it's out, without needing something especially from it.

So I'm running 2.23.1 and I'm interested in conservative, stabilization updates. Clearly 'stable_branch_release' is what I should select.

The previous comment stated that I didn't manage to get any 2.24 notification with this option selected.

Then, in the last part of the comment, I asked the question: if I'm running a devel version (2.23.1), if I'm having the objective described above (conservative upgrades down the 2.24 route), and if I've selected "stable_branch_release", should I receive notification about 2.23.2? How about 2.24-rc0?

Comment 38

[Vlad Dascalu]

Oh, ok, and to be even more clear:

The reason for review- on v5 is:

-> lack of notice that it's an admin-only notification.

-> devel versions *never* get update notifications if they've selected 'stable_branch_release'. If I understood in a wrong way what stable_branch_release does (and you don't like my interpretation from the previous comments), it should reject the value with a message "you can't select stable_branch_release when you're running a development version".

Comment 39

[Frédéric Buclin]

(In reply to comment #36)
> I failed to get an update notification for the following scenario:
> 
> I modified the XML to point out 2.24.2 as the current stable release. I've
> selected the "stable_branch_release" option, and then I've tested it with the
> current version "2.23.1+". I didn't manage to get the notification for 2.24.

Of course! That's so by design and I don't want to change this behavior. 2.23.1+ doesn't belong to any branch but the trunk. And the trunk remains the trunk, even when a branch is created. I'm not going to change this behavior. Moreover, how could we know that we will create 2.24 instead of 3.0? Users using development snapshots should know what they are doing. Or they shouldn't play with development snapshots.


> I suspect the reason is the fact that $branch_version is "2.23", and then when
> it greps for that it doesn't find the 2.24 information. We should special-case
> numbers that are not divisible with 2 :)

No, for the reason given above.


> -> the red box needs to say something in the lines of "This is an admin-only
> notice". I kind of imagined Mitchell coming to b.m.o., seeing the box due to
> her admin privs, and then screaming: "justdave, what's up with this red stuff
> on top of the b.m.o. homepage?" :-)

You should read carefully. The red box already displays:
"This message is only shown to logged in users with admin privs."


> -> we need to establish what to do if we're running 2.23.1, 2.23.2 is
> available, and "stable_branch_release" is selected. Or what to do when 2.24-rc1
> is released (the rest of the scenario is the same).

As said, you are not running a release from a branch, but from the trunk. And the trunk always exists so "stable_branch_release" won't return anything in this case, as expected.



(In reply to comment #37)
> I'm running 2.23.1 (because it has some very cool feature, very important for
> me, that 2.22 lacks). I don't want to go forever with devel, in fact I want to
> be conservative and keep the safest path: go down the 2.24 route. I don't even
> want 2.26/3.0 when it's out, without needing something especially from it.

So what you want is "latest_stable_release", not "stable_branch_release". Admins using a development snapshot should understand that they are running a release from the *trunk*.


> So I'm running 2.23.1 and I'm interested in conservative, stabilization
> updates. Clearly 'stable_branch_release' is what I should select.

See my previous comment.


> The previous comment stated that I didn't manage to get any 2.24 notification
> with this option selected.

Because you selected the wrong option.


> Then, in the last part of the comment, I asked the question: if I'm running a
> devel version (2.23.1), if I'm having the objective described above
> (conservative upgrades down the 2.24 route), and if I've selected
> "stable_branch_release", should I receive notification about 2.23.2? How about
> 2.24-rc0?

None of them, see my previous comments.


(In reply to comment #38)
> -> lack of notice that it's an admin-only notification.

There is such a notice already. On both editparams.cgi and index.cgi.


> -> devel versions *never* get update notifications if they've selected
> 'stable_branch_release'.

That's because you should use "latest_stable_release'".


> If I understood in a wrong way what
> stable_branch_release does (and you don't like my interpretation from the
> previous comments), it should reject the value with a message "you can't select
> stable_branch_release when you're running a development version".

Sure, I don't like your interpretation of "stable_branch_release"! :)

Comment 40

[Frédéric Buclin]

Attachment: patch, v6

vladd, here is a compromise which should make us both happy. I didn't change anything in the logic of my code, but I added an error message when the admin tries to select 'stable_branch_release' while he is running a development snapshot, as you suggested in your previous comment.

Let's hope this is the final revision of this patch.

Comment 41

[Colin Ogilvie [:cso]]

(In reply to comment #39)
> Of course! That's so by design and I don't want to change this behavior.
> 2.23.1+ doesn't belong to any branch but the trunk. And the trunk remains the
> trunk, even when a branch is created. I'm not going to change this behavior.
> Moreover, how could we know that we will create 2.24 instead of 3.0? Users
> using development snapshots should know what they are doing. Or they shouldn't
> play with development snapshots.

Hmm, I can see cases where it WOULD be useful for someone on a development version getting an update notification for a stable release... If an Admin wants $x feature, which is in 2.23.2 but then wants to upgrade to 2.24 and not 2.25 (or whatever comes next) because he only wants that new feature.

I'd like a way for this to be included, personally.

Comment 42

[Frédéric Buclin]

(In reply to comment #41)

> Hmm, I can see cases where it WOULD be useful for someone on a development
> version getting an update notification for a stable release... If an Admin
> wants $x feature, which is in 2.23.2 but then wants to upgrade to 2.24 and not
> 2.25 (or whatever comes next) because he only wants that new feature.
> 
> I'd like a way for this to be included, personally.

I answered this question already: select "latest_stable_release". When 2.24 is released, you will get a notification.

Comment 43

[Frédéric Buclin]

Attachment: patch, v6.1

Changing the default value to 'latest_stable_release'.

Comment 44

[Frédéric Buclin]

Attachment: patch, v7

Several changes as per our discussion on IRC. Use interdiff to see what changed.

Comment 45

[Vlad Dascalu]

Comment on attachment 225121 [details] [diff] [review]
patch, v7

According to http://publib.boulder.ibm.com/infocenter/iseries/v5r3/index.jsp?topic=/apis/utime.htm , you need to be the owner of the file in order to use utime on it.

I've had a setup in which "vladd" owned bugzilla-update.xml, and "apache" chgrp-ed it. Both had read and write permissions on it, but apache kept failing updating the timestamp. Per that documentation, you need to be the owner in order to update the timestamp with the utime thing.

So utime can fail, even if you have write access to the file.

So we have 3 possible solutions:

1) store the last time fetched timestamp somewhere else (not in the metadata). This involves checking permissions for this additional file as well.

2) update the timestamp if possible using something different than utime().

3) check the return value of utime(), and report errors in the interface (and stop fetching update.bugzilla.org until they are fixed).

Comment 46

[Vlad Dascalu]

Oh, and my error_log is also full of:

[Sat Jun 10 16:44:59 2006] [error] [client 127.0.0.1] [Sat Jun 10 16:44:59 2006] index.cgi: Operation not permitted at Bugzilla/Update.pm line 54.

Comment 47

[Vlad Dascalu]

I forgot to mention the reason for review-:

People do occasionally "chown -R theirUsername bugzilla" (because checksetup.pl doesn't let you edit the files after it's run). That was my case. With a setup like this one, utime() will fail to update the timestamp, and Bugzilla installations will start to DDoS the update server (after a week or so).

Comment 48

[Frédéric Buclin]

Attachment: patch, v7.1

One of the goals of checksetup.pl is to set permissions on files correctly. If you overwrite these permissions, that's not my problem anymore.

This patch fixes checksetup.pl to set permissions on bugzilla-update.xml to $webservergid, $webservergid so that your web server can use utime() on it.

Comment 49

[Frédéric Buclin]

From http://perldoc.perl.org/functions/utime.html:

"On most systems, this will set the file's access and modification times to the current time (i.e. equivalent to the example above) and will even work on other users' files where you have write permission"

So you don't even need to be the owner of the file. You only need write access to the file.

Comment 50

[Vlad Dascalu]

Wrong, I tested it and it didn't work.

utime() works by calling the C library, and if you look in the documentation that I pointed as an URL in comment 45, you'll see that it specifies the requirement of file ownership (as long as you don't use some strange filesystem that it mentions).

I'm puzzled how you can say that it works with me testing it and noticing that it doesn't.

Anyway, I'd prefer a new patch in which to consolidate the redundant code via a new sub (I'd call it get_version_array). Basically it should contain the redundant regexp that you're applying both to the current version and to the fetched version, and possibly the transformation code into the array (the RC thing and stuff).

That's purely optional and I'll upload a new version patch with it if LpSolit doesn't get to it first (sorry for the harasement, I'll try to make one big review one time only and to catch all the issues in order to avoid such incidents).

Comment 51

[Frédéric Buclin]

(In reply to comment #50)

> I'm puzzled how you can say that it works with me testing it and noticing that
> it doesn't.

I didn't say it was working in all cases, I was quoting what the Perl docs say. I hope you noticed that my last patch (v7.1) sets the web server group as the owner of the file...

Comment 52

[Frédéric Buclin]

Comment on attachment 225128 [details] [diff] [review]
patch, v7.1

glob, could you test this patch on Windows?

Comment 53

[Vlad Dascalu]

Comment on attachment 225128 [details] [diff] [review]
patch, v7.1

Ok, I'll try to do better.

I'm not worried at all that some idiot (like myself) could run chown -R vladd bugzilla, and ruin his permissions. I'm not worried at all that the idiot has a broken system due to this command until he runs checksetup.pl again.

The issue here, is the fact that the idiot will keep hitting the update.bugzilla.org server at every homepage hit, and that has the potential to DoS our update server.

Please understand that I agree in this situation with your thinking: idiots that chown things manually break their system and they should deserve that fate. What I don't agree is providing to those idiots a simple tool (without them probably being aware of it) that DDoS the update.bugzilla.org server with every homepage hit. That is unacceptable.

Please also address the code redundancy issues in the new patch.

Comment 54

[Vlad Dascalu]

Context from the chat:

<LpSolit> vladd: and idiots with admin privs don't go to index.cgi every 2 seconds
<vladd> LpSolit: idiots that do that intentionally get banned. I don't think we're going to ban thousands of users that break their system regularly in this way.

Comment 55

[Vlad Dascalu]

Comment on attachment 225128 [details] [diff] [review]
patch, v7.1

Ok, so LpSolit said that he gives up, and since I already felt push around for this review, I give up as well.

I've restored the original review flags. Have fun.

Comment 56

[Max Kanat-Alexander]

Comment on attachment 225128 [details] [diff] [review]
patch, v7.1

>Index: Bugzilla/Update.pm
< [snip]
>+# Look for new releases and notify logged in administrators about them.
>+sub get_notifications {
>+    return if (Bugzilla->params->{'upgrade_notification'} eq 'disabled');
>+
>+    # If the XML::Twig module is missing, we won't be able to parse
>+    # the XML file. So there is no need to go further.
>+    eval("require XML::Twig");
>+    return if $@;

  Nit: That could be a block eval.

>+    my $remote_file = 'http://updates.bugzilla.org/bugzilla-update.xml';

  Nit: That seems like it should be in a constant.

>+    my $xml_file = "$datadir/bugzilla-update.xml";

>+    # Update the local XML file if this one doesn't exist
>+    # or is older than a week (604'800 seconds).
>+    if (!-e $xml_file || (time() - (stat($xml_file))[9] > 604800)) {

  '604800' definitely needs to be in a constant.

  Also, add a comment about what stat($xml_file)[9] means, because I have no idea, offhand.

>+        # If we are unable to update the XML file, it doesn't make
>+        # sense to use it.
>+        eval("require LWP::UserAgent");
>+        return if $@;

  Nit: That could also be a block eval.

>+
>+        my $ua = LWP::UserAgent->new();
>+        $ua->timeout(5);

  That timeout should probably be in a constant.

>+    my @releases;
>+    foreach my $branch ($root->children('branch')) {
>+        my $release = {
>+            'branch_ver' => $branch->{'att'}->{'id'},
>+            'latest_ver' => $branch->{'att'}->{'vid'},
>+            'status'     => $branch->{'att'}->{'status'},
>+            'url'        => $branch->{'att'}->{'url'},
>+            'date'       => $branch->{'att'}->{'date'}
>+        };
>+        push(@releases, $release);
>+    }

  Couldn't that be a hash, with the branch as a key, instead of an array?

>+sub _compare_versions {

  We have a vers_cmp sub in checksetup.pl that you could move somewhere else. I was thinking of making a Bugzilla::Requirements module--that might be appropriate. Of course, I've noticed that you're using arrays here, so maybe it wouldn't be appropriate.

>+and parse it in order to display information based on your

  "and parses it," (add s and comma)

>+=item C<get_notifications()>
>+
>+ Description: This function inform you about new releases, if any.

  informs

>+ Returns:     A reference to a hash with data about new releases,
>+              if any, or the reason of the failure if the XML file
>+              containing all information about new releases cannot
>+              be parsed.

  "Reason of the failure?" Describe the returned hash.

>+        # The web server must be the owner of bugzilla-update.xml.
>+        fixPerms("$datadir/bugzilla-update.xml", $webservergid, $webservergid, 017);

  This works fine even if the file doesn't exist yet, right?

>Index: skins/standard/index.css
> [snip]
>@@ -66,4 +66,17 @@
>         padding: 1em 0;
>     }
> 
>+    #new_release
>+    {
>+        border: solid red;

  You didn't specify a border-width there. I think you have to actually specify the width somewhere. (As in "2px solid red".)

>+                          "<li>'latest_stable_release' notifies you about the most recent release " _
>+                          "available on the most recent stable branch, independently of your " _
>+                          "installation.</li>" _

  What do you mean by "independently of your installation?" I think I understand it, but only after a minute or so of thinking about it. Perhaps just use some more words instead of those.

>+                          "If you are running a release candidate, you will get notification for " _
>+                          "newer release candidates too.</li>" _

  "get a notification.

>+    [% ELSIF release.error == "unknown_parameter" %]
>+      <p>An invalid parameter has been passed. 

  To what?

Comment 57

[victory <[e-mail]>]

store those information into db and those pooor issue will have gone i think :-p

Comment 58

[Teemu Mannermaa (:wicked)]

(In reply to comment #45)
> (From update of attachment 225121 [details] [diff] [review] [edit])
> According to
> http://publib.boulder.ibm.com/infocenter/iseries/v5r3/index.jsp?topic=/apis/utime.htm
> , you need to be the owner of the file in order to use utime on it.

Note that this is documentation for APIs available on an IBM AS400/iSeries/i5 servers. It's possible that their implementation details differ from other systems. I haven't heard of anybody running Bugzilla on these systems, mostly because they don't come with a prebuild perl interpreter. I would love to, though. :)

(In reply to comment #49)
> From http://perldoc.perl.org/functions/utime.html:
> So you don't even need to be the owner of the file. You only need write access
> to the file.

Yup, but you need to call with two undef parameters for this to effective. Right now you call it with result of utime(). This is redundant per docs and probably is what vladd is seeing. Maybe this should be changed?

LpSolit, will you update this patch with mkanat's comments or should somebody else take it?

Comment 59

[Frédéric Buclin]

Attachment: patch, v8

/me had to sleep.

OK, here is an updated patch which makes sure the web server is able to change the XML file timestamp before trying to update it. If it cannot change its timestamp, an warning is displayed. (this fixes vladd's complaint)

Moreover, I also included most comments mkanat made. I'm fine with the way I use eval() here, so I didn't change them.

Comment 60

[Max Kanat-Alexander]

Comment on attachment 225189 [details] [diff] [review]
patch, v8

  This looks great to me. :-)

  Except:

>+use constant LOCAL_FILE    => "$datadir/bugzilla-update.xml";

  That won't work. Constants can't contain variables. You could just make the *name* of the file a constant, and append it to $datadir inside of the function.

Comment 61

[Frédéric Buclin]

Attachment: patch, v8.1

Fixing mkanat's comment. My installation didn't complain about mixing variables in a constant; strange.

Carrying mkanat's r+ forward (I asked on IRC).

Comment 62

[Vlad Dascalu]

Comment on attachment 225216 [details] [diff] [review]
patch, v8.1

<a href="[% release.data.url FILTER html %]">

We need FILTER url_quote here, as Colin suggested in comment 27.

+    my @current_version =
+        ($Bugzilla::Config::VERSION =~ m/^(\d+)\.(\d+)(?:(rc|\.)(\d+))?\+?$/);

+    my @new_version =
+        ($release[0]->{'latest_ver'} =~ m/^(\d+)\.(\d+)(?:(rc|\.)(\d+))?\+?$/);
+
+    # We convert release candidates 'rc' to integers (rc ? 0 : 1) in order
+    # to compare versions easily.
+    $current_version[2] = ($current_version[2] && $current_version[2] eq 'rc') ? 0 : 1;
+    $new_version[2] = ($new_version[2] && $new_version[2] eq 'rc') ? 0 : 1;

This duplicated code didn't get removed.

Comment 63

[Frédéric Buclin]

(In reply to comment #62)
> (From update of attachment 225216 [details] [diff] [review] [edit])
> <a href="[% release.data.url FILTER html %]">
> 
> We need FILTER url_quote here, as Colin suggested in comment 27.

No, FILTER html is correct, see bug 305807 comment 1.

Comment 64

[Frédéric Buclin]

Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.484; previous revision: 1.483
done
Checking in index.cgi;
/cvsroot/mozilla/webtools/bugzilla/index.cgi,v  <--  index.cgi
new revision: 1.18; previous revision: 1.17
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Update.pm,v
done
Checking in Bugzilla/Update.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Update.pm,v  <--  Update.pm
initial revision: 1.1
done
Checking in Bugzilla/Config/Common.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Config/Common.pm,v  <--  Common.pm
new revision: 1.6; previous revision: 1.5
done
Checking in Bugzilla/Config/Core.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Config/Core.pm,v  <--  Core.pm
new revision: 1.4; previous revision: 1.3
done
Checking in skins/standard/index.css;
/cvsroot/mozilla/webtools/bugzilla/skins/standard/index.css,v  <--  index.css
new revision: 1.4; previous revision: 1.3
done
Checking in template/en/default/index.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/index.html.tmpl,v  <--  index.html.tmpl
new revision: 1.26; previous revision: 1.25
done
Checking in template/en/default/admin/params/core.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/params/core.html.tmpl,v  <--  core.html.tmpl
new revision: 1.3; previous revision: 1.2
done

Comment 65

[Frédéric Buclin]

Attachment: documentation patch, v1

This patch doesn't add the "upgrade_notification" parameter to the list of parameters. As 90% of parameters are missing there, I prefer to add them all at once in a separate bug. So this patch mainly only mentions that such a feature exists.

Comment 66

[Dave Miller [:justdave]]

Comment on attachment 246503 [details] [diff] [review]
documentation patch, v1

>+        identified and closed), a bugfix release is made. As an
>+        example, 2.20.2 was a bugfix release, and improved on 2.20.3.

You have these two backwards.  2.20.3 was a bugfix release and improved on 2.20.2.

>@@ -1861,19 +1986,18 @@ P template/en/default/list/quips.html.tm
>-bash$ <command>wget ftp://ftp.mozilla.org/pub/mozilla.org/webtools/bugzilla-2.18.1.tar.gz</command>
>+bash$ <command>wget ftp://ftp.mozilla.org/pub/mozilla.org/webtools/bugzilla-2.22.1.tar.gz</command>

As long as we're changing this, can we make the protocol be http instead of ftp?  We've got more options server-side for enhancing the performance of the downloads that way, and it also reminds me that I didn't include any ftp download counts in those stats I pulled a few weeks ago. :)

>-bash$ <command>wget ftp://ftp.mozilla.org/pub/mozilla.org/webtools/bugzilla-2.18.0-to-2.18.1.diff.gz</command>
>+bash$ <command>wget ftp://ftp.mozilla.org/pub/mozilla.org/webtools/bugzilla-2.22-to-2.22.1.diff.gz</command>
> <emphasis>(Output omitted)</emphasis>

Ditto here.

Otherwise, looks good.

Comment 67

[Frédéric Buclin]

Attachment: documentation patch, v1.1

Comment 68

[Frédéric Buclin]

Checking in docs/xml/administration.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/administration.xml,v  <--  administration.xml
new revision: 1.69; previous revision: 1.68
done

Comment 69

[Max Kanat-Alexander]

Added to the release notes in bug 349423.