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

[#12494] Allow instructors to use start/end dates up to 12 months in future #12498

Merged
merged 24 commits into from
Jul 12, 2023

Conversation

Zxun2
Copy link
Contributor

@Zxun2 Zxun2 commented Jun 29, 2023

Fixes #12494

Outline of solution

  • Update date range from 3 months to 12 months
  • Update comments where necessary
  • Add safety net in cases where start date is later than the end date. In which case, the end date automatically defaults to the current start date.

Demo

Safety.net.and.date.range.for.Instructor.Sessions.mp4

@domlimm domlimm added the s.ToReview The PR is waiting for review(s) label Jun 30, 2023
@domlimm domlimm self-requested a review June 30, 2023 03:57
@weiquu
Copy link
Contributor

weiquu commented Jul 2, 2023

Hi @Zxun2, I'm unable to save a session if I set the start date to be 12 months later... can you take a look? This occurs both when creating a new session and editing a session:

Screenshot 2023-07-03 at 1 08 11 AM

Add safety net in cases where start date is later than the end date. In which case, the end date automatically defaults to the current start date.

Not sure if you want to include time in it as well, or if you think it's not needed:
Screenshot 2023-07-03 at 1 04 23 AM

Copy link
Contributor

@cedricongjh cedricongjh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a few comments, and also I would like to suggest that it might be better from a UX perspective to prevent selection of dates/times on the frontend, rather than surfacing invalid datetimes through the BE as error toasts, but this could be in a seperate PR.

@weiquu
Copy link
Contributor

weiquu commented Jul 6, 2023

Did some tests, but haven't taken a detailed look at the code! Just one comment:

  • Set opening to 13 Sep 1800h and closing to 15 Sep 1600h => set opening to 16 Sep => closing date changes but time doesn't change, leading to the following:
Screenshot 2023-07-07 at 1 43 58 AM

Think just needs a quick change (: Other than that the behaviour looks good

@weiquu weiquu requested a review from cedricongjh July 8, 2023 08:12
Copy link
Contributor

@weiquu weiquu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 small nits! Other than that it looks great (: Pinging @domlimm and @cedricongjh for review

Copy link
Contributor

@cedricongjh cedricongjh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, once the nits by @weiquu are fixed!

Copy link
Contributor

@weiquu weiquu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Edit: sorry just spotted a small bug....... will re-review in a bit

@weiquu weiquu self-requested a review July 11, 2023 10:51
Copy link
Contributor

@weiquu weiquu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One small edge case to fix!

Copy link
Contributor

@weiquu weiquu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@weiquu weiquu merged commit 930c2b1 into TEAMMATES:master Jul 12, 2023
@damithc
Copy link
Contributor

damithc commented Jul 13, 2023

@samuelfangjw @zhaojj2209 can we do a release with this feature? I'm receiving more user inquiries about this restriction.

@samuelfangjw samuelfangjw removed the s.ToReview The PR is waiting for review(s) label Jul 14, 2023
@samuelfangjw samuelfangjw added this to the V8.28.0 milestone Jul 14, 2023
@samuelfangjw samuelfangjw added s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging c.Feature User-facing feature; can be new feature or enhancement to existing feature labels Jul 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c.Feature User-facing feature; can be new feature or enhancement to existing feature s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow instructors to use start/end dates up to 12 months in future
6 participants