1156738 - It is possible to create a yearly rule with an invalid day

Status: RESOLVED FIXED

(Calendar :: Dialogs, defect)

Description

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

STR:

1. Open custom recurrence dialog
2. Switch to annually, every <num> <month>
3. Change <num> to 63

Expected:

number should max at 31 and possibly depend on the selected month.

Comment 1

[Rashed]

Hello, I'd like to fix this bug as my first bug.
I did setup the build environment for Firefox, and any further mentoring would be great!

Comment 2

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Heya, glad to hear you would like to work on this bug. You mentioned you've set up a Firefox environment, was this just a mixup? This bug is about Lightning, which is an extension to Thunderbird. To work on this bug you should set up a build environment for Thunderbird following https://developer.mozilla.org/en-US/docs/Simple_Thunderbird_build

If you have indeed set up a Firefox environment, you can speed up the process for setting up a Thunderbird environment a bit. Before you execute python client.py checkout, you can link your mozilla-central clone into the mozilla directory in comm-central, i.e. (cd ~/mozilla/comm-central; ln -s ../mozilla-central mozilla) given your repositories are both in ~/mozilla.

You can use dxr.mozilla.org/comm-central for code search (or mxr.mozilla.org if you prefer that interface), the relevant file is https://dxr.mozilla.org/comm-central/source/calendar/base/content/dialogs/calendar-event-dialog-recurrence.xul

Comment 3

[Decathlon]

Since the patch should be simple and probably it doesn't need to build, you could also work directly on Lightning's code inside Thunderbird's profile folder and then copy the changes on your comm-central cloned repository only to make a patch file.
If you do in this way please download and install the latest versions of TB and LG for your platform:
http://ftp.mozilla.org/pub/mozilla.org/thunderbird/nightly/latest-comm-central/
http://ftp.mozilla.org/pub/mozilla.org/calendar/lightning/nightly/latest-comm-central/
(for Windows you can use the zip version of Thunderbird without installer).

Comment 4

[Rashed]

(In reply to Philipp Kewisch [:Fallen] from comment #2)
> You mentioned you've set up a Firefox environment, was this just a mixup? This bug is about
> Lightning, which is an extension to Thunderbird.

Sorry, my fault, but I still like to work on this bug though.
I'm currently setting up ThunderBird's build environment.

Comment 5

[Rashed]

(In reply to Decathlon from comment #3)
> Since the patch should be simple and probably it doesn't need to build, you
> could also work directly on Lightning's code inside Thunderbird's profile
> folder and then copy the changes on your comm-central cloned repository only
> to make a patch file.
> If you do in this way please download and install the latest versions of TB
> and LG for your platform:
> http://ftp.mozilla.org/pub/mozilla.org/thunderbird/nightly/latest-comm-
> central/
> http://ftp.mozilla.org/pub/mozilla.org/calendar/lightning/nightly/latest-
> comm-central/
> (for Windows you can use the zip version of Thunderbird without installer).

Well, I tried to play around a little in "calendar-event-dialog-recurrence.js" file in the default profile, but couldn't see the results reflected.

Comment 6

[Decathlon]

For the main part of the bug (a generic upper limit for a textbox) you need to work only in calendar-event-dialog-recurrence.xul file, instead for a limit that depends on the selected month maybe you need to work on 
calendar-event-dialog-recurrence.js too.

(In reply to A.Rahman Rashed from comment #5)

> Well, I tried to play around a little in
> "calendar-event-dialog-recurrence.js" file in the default profile, but
> couldn't see the results reflected.

The effects take place when you restart Thunderbird (also be sure to work in the right profile if you have more than one ;-)

Comment 7

[Rashed]

(In reply to Decathlon from comment #6)
> For the main part of the bug (a generic upper limit for a textbox) you need
> to work only in calendar-event-dialog-recurrence.xul file, instead for a
> limit that depends on the selected month maybe you need to work on 
> calendar-event-dialog-recurrence.js too.
> 
> (In reply to A.Rahman Rashed from comment #5)
> 
> > Well, I tried to play around a little in
> > "calendar-event-dialog-recurrence.js" file in the default profile, but
> > couldn't see the results reflected.
> 
> The effects take place when you restart Thunderbird (also be sure to work in
> the right profile if you have more than one ;-)

Hello Decathlon, I thought instead of setting 'max' attribute of "yearly-days" textbox I might limit the value according to the selected month, so I guess I'd be working on calendar-event-dialog-recurrence.js file, what do you think ?

It turned out I've been updating the wrong profile, thanks alot :)

Comment 8

[Rashed]

I tried setting the limit according to the selected month, within the function onSave(item), in the switch clause, "yearly" case and it worked.
Should I continue ?

Comment 9

[Decathlon]

If you have a code that works, you can create a patch and attach it here (see the link above), and set the reviewer, so we can take a look :)

Comment 10

[Rashed]

Attachment: fixes the invalid day entry for yearly rules in Lightning

Comment 11

[Stefan Sitter [:ssitter]]

Hello A.Rahman Rashed, thanks for working on this bug report. A quick note about some things you might want to test with your patch: 
Is it not unexpected when Lightning saves something different than what was entered and visible to the user in the UI?
What happens when you edit an existing event with invalid day that was created before that fix?
What happens when you try to create an event on event on February 29th in leap years?

Comment 12

[Rashed]

(In reply to Stefan Sitter from comment #11)
> Hello A.Rahman Rashed, thanks for working on this bug report. A quick note
> about some things you might want to test with your patch: 

Great, these are the results of my test:

> Is it not unexpected when Lightning saves something different than what was
> entered and visible to the user in the UI?

I believe that the consistency between entered value and the UI is preserved, the edit in the patch prevents the user from even proceeding with an invalid day, it automatically sets the value of textbox to the max value of days in a month.

> What happens when you edit an existing event with invalid day that was
> created before that fix?

For invalid day, it gets set with the max value that day can get in that particular month (last day of month) whenever he edits the rule.

> What happens when you try to create an event on event on February 29th in
> leap years?

Unfortunately, I didn't handle that case, but I'm a little bit confused about it, should the user always be able to set an event on 29th of February, or should it depend on the period of the event (including starting and ending date) ?

Comment 13

[Decathlon]

Comment on attachment 8600677 [details] [diff] [review]
fixes the invalid day entry for yearly rules in Lightning

In order to create a patch that makes easier to push it in the repository, you should add the options for current user and current date, this last one also for qrefresh, in your mercurial configuration file (.hgrc):

[defaults]
qnew = -D -U
qrefresh = -D

you should also add a commit message with the number and the summary of the bug (copy and paste the summary at the top of this bugzilla page "Bug 1156738 - Its possible to ...") and the reviewer "r=..." at the end.
In this way mercurial generates a patch with a useful header ready to push into comm-central with the correct informations when it is finished.

The code style needs an adjustment. Please follow this guide line:
https://wiki.mozilla.org/Calendar:Style_Guide
generally, you have to adapt the style to the rest of the code in the file where you are working.
In this case you have to change: spaces instead of tabs to indent the code; brackets even for one code line included in "if", "for", ... blocks; leave spaces around -+/* operators and between "if" and "(".


The patch works fine for a new event or an edited one with a valid rule, it only needs to set the maximum number of day for February to 29 independently from the year (leap or not) because it has to be always possible to create a yearly recurrence for the 29th of February even when the day is not present in a particular year.
At the moment there's a problem about how Lightning interprets the 29th of February for not leap years but this is already tracked by bug 160821.

Setting r- for now for these reasons but wait before attach a new patch because we have also to consider the issue pointed out by Stefan in comment #11 about the invalid rules created with a wrong day of the month.

Comment 14

[Decathlon]

About editing an existing event with invalid day that was previously created by Lightning (comment #11), we have to take a decision.
An invalid rule e.g. the *34th of May* :

RRULE:FREQ=YEARLY;BYMONTH=5;BYMONTHDAY=34

is parsed and treated by libical/icaljs and displayed in the views as the *3th of June* because the date gets normalized in the next month(s): 3 = 34-31). If we accept this behavior, we have to make consistent the informations provided by the dialogs (Edit and Recurrence dialogs).
The patch transforms a such rule into the *31st of May* and saves the rule with BYMONTHDAY=31 when the dialog is closed with the OK button and this is not correct.

Not sure what should be the right way to proceed, but we could intercept these rules type (YEARLY+BYMONTH+BYMONTHDAY) with invalid days when opening the Edit Dialog in the calendar-event-dialog.js file, and then normalize the date and change the rule before doing any other elaboration in the dialogs.
This means that the dialog will ask to save before close even when nothing has been changed by the user, but since the rule is invalid (or better, is written in wrong way) I think this is acceptable.
Philipp, what's your opinion?

Comment 15

[Rashed]

Having taken these points into consideration; the next patch will be much more organized, thanks a lot Decathlon for pointing these issues.
I'm waiting for a decision to be taken about the existing invalid dates to continue working on the patch.

Comment 16

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

(In reply to Decathlon from comment #14)

> This means that the dialog will ask to save before close even when nothing
> has been changed by the user, but since the rule is invalid (or better, is
> written in wrong way) I think this is acceptable.
> Philipp, what's your opinion?

If the custom recurrence dialog is cancelled, couldn't we revert to the original rule? If that is too complicated I'm fine with leaving it as you suggested, the user can still cancel the event dialog to revert the changes.

Comment 17

[Decathlon]

(In reply to Philipp Kewisch [:Fallen] from comment #16)
> (In reply to Decathlon from comment #14)

> If the custom recurrence dialog is cancelled, couldn't we revert to the
> original rule? If that is too complicated I'm fine with leaving it as you
> suggested, the user can still cancel the event dialog to revert the changes.

I think the conversion of the rule must be done when opening the Edit dialog, i.e. before opening the custom Recurrence dialog because there is the string that summarizes the rule that also needs the correct day and month otherwise we can have absurd sentences like currently happens: "Occurs every May 45 effective 13/5/2015 ..."
Otherwise we can find the correct day and month and use these parameters only for displaying the string and the controls in the dialog without changing the rule, but it seems even more complicated.


Rashed, I don't know how difficult it could be. If you want to try please go ahead, otherwise we can keep your patch and open another bug for this specific issue that blocks this one.

Comment 18

[Rashed]

(In reply to Decathlon from comment #17)
 
> Rashed, I don't know how difficult it could be. If you want to try please go
> ahead, otherwise we can keep your patch and open another bug for this
> specific issue that blocks this one.

I will work on it, but I think that opening another bug would be better in case I couldn't get it done.
About the old patch, should I upload a new patch with the edits you mentioned before ? also I noticed that I'm not yet assigned to the bug.

Thanks Philipp and Decathlon :)

Comment 19

[Decathlon]

(In reply to Rashed from comment #18)
> I will work on it, but I think that opening another bug would be better in
> case I couldn't get it done.

Don't worry, this bug is an old one despite it has been open only recently so we can wait before open a new "sub-bug". Take your time to figure out how to solve it, ask if you have doubts, also by email :)

> About the old patch, should I upload a new patch with the edits you
> mentioned before ? also I noticed that I'm not yet assigned to the bug.

If you try to work on the "complete" bug, you have anyway to sort out the patch you've already attached, so you can attach only the edited patch if, at the end, you will decide to not fix the complete bug, but I think you can do easily the complete work ;)

Comment 20

[Pas]

Hi, I am new to fixing bugs and I would like to work on the bug please.

Comment 21

[saptak013]

I have done it to a max of 31 days. But I can't figure it out how to make it to the max number of days in the month selected. Can you help me out?

Comment 22

[Alex]

Is anyone actively working on this? I could take a quick look at this if not.

Comment 23

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Given the last comment was a year ago I suspect not. Let us know if you need more information!

Comment 24

[meghasharma4910]

I guess that this bug is still open, can I take it up?

Comment 25

[ds31]

I would like to be assigned this bug!

Comment 26

[Devika Sugathan]

Hello,
I'm an absolute beginner and would like to take up this bug.

Comment 27

[akanksha bhardwaj]

hello! is someone working on this bug, if not i would like to be assigned on it.

Comment 28

[Paarmita Bhargava]

Hey,
I would like to work on it. Can you please assign it to me?

Comment 29

[sujaytandel.proj]

Hey,
Is the issue still open? If so then can I take up this bug as my good-first-bug?

Comment 30

[snikithasiddavatam]

Hey! Would like to work on this as my first bug! Would like your mentoring going forward.

Comment 31

[Manikant Kumar]

hey I would like to work on this bug as this is my first bug Would like your mentoring going forward.

Comment 32

[Leonardo Nobrega]

Attachment: bug1156738.patch

Hello, new developer here. I'd like to work on this bug.

Comment 33

[Gunjan Yadu]

I am new to open source. I would like to work on this issue. Can I take it? Thanks:)

Comment 34

[Kajal Sah]

Looks like this bug is not fixed yet. Can I work on this issue? If yes, I would appreciate some suggestions on solving this bug.

Comment 35

[Wayne Mery (:wsmwk)]

(In reply to Kajal Sah from comment #34)

Looks like this bug is not fixed yet. Can I work on this issue? If yes, I would appreciate some suggestions on solving this bug.

https://www.thunderbird.net/en-US/get-involved/ gives some places to look for starting information.

Comment 36

[anayocrescent2]

hello, I would like to make a contribution to this bug, how do I get assigned and started

Comment 37

[Kian]

Attachment: Bug 1156738 - Fixed: It is possible to create a yearly rule with an invalid day. r=wsmwk

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/4020f624ad7f
Fixed: It is possible to create a yearly rule with an invalid day. r=wsmwk