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

Feature/standard times issue 300 #301

Closed
wants to merge 6 commits into from

Conversation

rpruim
Copy link
Collaborator

@rpruim rpruim commented Nov 28, 2022

As per #300, I've made the standard time check a bit looser. This will allow people to schedule classes that meet only one time from a meeting pattern without being flagged.

I've implemented this be expanding the list of legal meeting patterns when it is created to include the one-day versions of teach meeting pattern. Everything else is just as before.

I've added several additional test cases to confirm the new behavior is working as desired.

Side note: See #295. This would be cleaner if I didn't need to convert from TH to R and back again.

Copy link
Collaborator

@kvlinden kvlinden left a comment

Choose a reason for hiding this comment

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

LGTM

@rpruim
Copy link
Collaborator Author

rpruim commented Nov 30, 2022

I'm confused about this PR. It appears the changes it is intended to introduce are already in production. Furthermore, there is a conflict with a subsequent update to the standard meeting patterns. I've twice tried to resolve those on github, but they don't seem to take.

I'm going to close this since I don't think it is actually needed.

@rpruim rpruim closed this Nov 30, 2022
@charkour
Copy link
Collaborator

charkour commented Dec 1, 2022

It appears the changes it is intended to introduce are already in production.

Did you push to production on accident at one point? Or were they included with a different feature branch? It seems the latter is most likely (I've done that before).

@rpruim
Copy link
Collaborator Author

rpruim commented Dec 1, 2022

I don't think I can push to production. (I tried it once to see if that's what happened and my push was blocked.)

I'm not sure just what happened, but if you are interested, this is roughly how things went down (IIRC):

  • I used a branch to make a pull request, then I continued to work in that branch so I could update the PR.
  • Meanwhile @kvlinden approved the PR (and I may have merged it at that point, but I don't remember exactly).
  • Github cleans up old branches involved in PRs, so when I tried to push to my branch, it was longer on the remote.
  • So I made a new branch at that commit and pushed that and issued a PR that had mostly the same commits, but another added on.
  • Meanwhile, we approved and merged a different PR to fix the data in the meeting patterns, that caused the this second PR to have conflicts.
  • I tried resolving them on github (because I wanted to see how that worked), and I clicked on resolved, but the resolution didn't seem to persist. In fact, I did that twice.

@charkour
Copy link
Collaborator

charkour commented Dec 5, 2022

Ah yeah, sounds like a complicated git workflow. I wish GitHub had the option to disallow fixing conflicts online. Often times, it doesn't work quite right.

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.

3 participants