Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: dummy value used if current time 0h:0m - fajr #5

Closed
wants to merge 1 commit into from

Conversation

Moanrisy
Copy link

Bug: current_prayer is Dohr when it should be Isyaa after midnight (0h:0m)

@azzamsa
Copy link
Owner

azzamsa commented Jul 25, 2023

As I mentioned before in #3 (comment), please check if my patch works for you.

@Moanrisy
Copy link
Author

Moanrisy commented Jul 25, 2023

Sorry I forget to mention my test on that patch.

The patch still not working in my case, this is the screenshots

This is the first bug when happened after isya #4
time after isya

This patch still showing 0:0 if Current Prayer is Isya

This is the second bug when happened after midnight (the bug in this PR) #5
time after midnight

@azzamsa
Copy link
Owner

azzamsa commented Jul 26, 2023

Yes. The patch contains only one commit for the second bug you described here #3 (comment).

Is that patch solves your second bug?

@Moanrisy
Copy link
Author

Moanrisy commented Jul 26, 2023

It still not solved yet using the patch.

Using the patch, when the current time is between 0:0 until fajr time it show Error: InvalidTime.

I think the use of dummy value is still nessecarry.
On the patch you remove the dummy value with

    fn current_time(&self, time: DateTime) -> Result<Prayer, crate::Error> {
        let mut current_prayer: Option<Prayer> = None;

But the problem is fixed by changing the dummy value from Dohr to Isyaa
let mut current_prayer = Prayer::Dohr; -> let mut current_prayer = Prayer::Isyaa

@azzamsa
Copy link
Owner

azzamsa commented Jul 26, 2023

Using the patch, when the current time is between 0:0 until fajr time it show Error: InvalidTime.

Would you like to give me the reproducible test cases? Such as https://github.com/azzamsa/islam/blob/master/src/salah/times.rs#L429-L437. I will add it to the test cases.

@Moanrisy
Copy link
Author

Moanrisy commented Jul 26, 2023

Finally I think I found the bug source.

ranges is used to determine current_prayer

let ranges = vec![
            // fajr, fajr_range
            (Prayer::Fajr, self.fajr..self.sherook),
            (Prayer::Sherook, self.sherook..self.dohr),
            (Prayer::Dohr, self.dohr..self.asr),
            (Prayer::Asr, self.asr..self.maghreb),
            (Prayer::Maghreb, self.maghreb..self.ishaa),
bug here ->            (Prayer::Ishaa, self.ishaa..self.fajr_tomorrow),
        ];

but the last item on ranges is never reach out.
on time.rs, now() is used to get current time

pub fn now() -> DateTime {
    Local::now().naive_local()

So if right now is 26 Jul 2023 at 20:23 time. The current prayer is ishaa.

But if the time change to 27 Jul 2023 at 01:23 time. The current prayer is not match with every item on ranges.
Because
(Prayer::Ishaa, self.ishaa..self.fajr_tomorrow),
Is always set to today time.

failures:
---- salah::times::tests::current_prayer_is_ishaa stdout ----
d: current time: 2023-07-26T 01:24:00

d: 2023-07-26T 04:42:54..2023-07-26T 06:04:20 -- NOT MATCH
d: 2023-07-26T 06:04:20..2023-07-26T 11:59:08 -- NOT MATCH
d: 2023-07-26T 11:59:08..2023-07-26T 15:21:18 -- NOT MATCH
d: 2023-07-26T 15:21:18..2023-07-26T 17:53:56 -- NOT MATCH
d: 2023-07-26T 17:53:56..2023-07-26T 19:06:54 -- NOT MATCH
d: 2023-07-26T 19:06:54..2023-07-27T 04:42:54 -- NOT MATCH
Error: InvalidTime

d: 2023-07-26T 19:06:54..**2023-07-27**T 04:42:54 -- NOT MATCH
the date with bold and italic never checked. Because after midnight, the current date will change too.

@Moanrisy
Copy link
Author

Moanrisy commented Jul 26, 2023

Using the patch, when the current time is between 0:0 until fajr time it show Error: InvalidTime.

Would you like to give me the reproducible test cases? Such as https://github.com/azzamsa/islam/blob/master/src/salah/times.rs#L429-L437. I will add it to the test cases.

    #[test]
    fn current_prayer_is_ishaa_after_midnight() -> Result<(), crate::Error> {
        // Ishaa is: 2021-04-19T19:00:27+07:00
        let date = time::date(2021, 4, 19)?;
        let config = Config::new().with(Method::Singapore, Madhab::Shafi);
        let prayer_times = prayer_times_with_date(config, date)?;
        let tomorrow_date = time::date(2021, 4, 20)?;
// test for is_ishaa is pass using date variable
// test of is_ishaa_after_midnight only pass if using tomorrow_date variable
        let current_prayer_time = expected_time_with_date(tomorrow_date, 01, 24, 0)?;

        assert_eq!(
            prayer_times.current_time(current_prayer_time)?,
            Prayer::Ishaa
        );
        Ok(())
    }

So my solution is to add another if case on ranges variable in current_time function

        for (prayer, range) in ranges {
            if range.contains(&time) {
                println!("MATCH");
                current_prayer = Some(prayer);
            }

            let tomorrow = Days::new(1);
            if range.contains(&time.checked_add_days(tomorrow).unwrap()) {
                current_prayer = Some(prayer);
            }
        }

but I'm using unwrap on that solution, so I'm not sure what is the best solution

@azzamsa
Copy link
Owner

azzamsa commented Sep 1, 2023

I use this approach instead.

        // Special case for time after 00:00
        // It never get any matching prayer in the iteration above
        if current_prayer.is_none() && time < self.fajr {
            current_prayer = Some(Prayer::Ishaa)
        }

https://github.com/azzamsa/islam/blob/master/src/salah/times.rs#L364-L368

@azzamsa azzamsa closed this Sep 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants