Closed Bug 1156738 Opened 7 years ago Closed 4 months ago

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

Categories

(Calendar :: Dialogs, defect)

defect
Not set
normal

Tracking

(thunderbird_esr91 wontfix)

RESOLVED FIXED
98 Branch
Tracking Status
thunderbird_esr91 --- wontfix

People

(Reporter: Fallen, Assigned: eghbalniakian)

Details

Attachments

(3 files)

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.
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!
Flags: needinfo?(bv1578)
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
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).
Flags: needinfo?(bv1578)
(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.
(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.
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 ;-)
(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 :)
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 ?
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 :)
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?
(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 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.
Attachment #8600677 - Flags: review?(bv1578) → review-
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?
Flags: needinfo?(philipp)
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.
(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.
Flags: needinfo?(philipp)
(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.
(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 :)
(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 ;)
Assignee: nobody → abduorashed
Hi, I am new to fixing bugs and I would like to work on the bug please.
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?
Is anyone actively working on this? I could take a quick look at this if not.
Given the last comment was a year ago I suspect not. Let us know if you need more information!
Assignee: abduorashed → nobody
I guess that this bug is still open, can I take it up?
I would like to be assigned this bug!
Hello,
I'm an absolute beginner and would like to take up this bug.
hello! is someone working on this bug, if not i would like to be assigned on it.

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

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

Flags: needinfo?(philipp)
Keywords: good-first-bug
Whiteboard: [good first bug][lang=js][lang=html] → [lang=js][lang=html]

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

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

Attached patch bug1156738.patchSplinter Review

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

Mentor: bv1578

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

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.

(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.

Flags: needinfo?(philipp)
Summary: Its possible to create a yearly rule with an invalid day → It is possible to create a yearly rule with an invalid day
Whiteboard: [lang=js][lang=html] → [lang=js][lang=html][patchlove]

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

Assignee: nobody → eghbalniakian
Status: NEW → ASSIGNED
Attachment #9254970 - Attachment description: Bug 1156738 - Fixed: It is possible to create a yearly rule with an invalid day. r=wsmwk → Bug 1156738 - Fixed: It is possible to create a yearly rule with an invalid day. r=darktrojan
Attachment #9254970 - Attachment description: Bug 1156738 - Fixed: It is possible to create a yearly rule with an invalid day. r=darktrojan → Bug 1156738 - Fixed: It is possible to create a yearly rule with an invalid day. r=wsmwk

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/4020f624ad7f
Fixed: It is possible to create a yearly rule with an invalid day. r=wsmwk

Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → 98 Branch
Keywords: good-first-bug
Whiteboard: [lang=js][lang=html][patchlove]
You need to log in before you can comment on or make changes to this bug.