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 pre-survey bug #35179

Merged
merged 2 commits into from Jun 5, 2020
Merged

Fix pre-survey bug #35179

merged 2 commits into from Jun 5, 2020

Conversation

molly-moen
Copy link
Contributor

When a teacher goes to /pd/workshop_pre_survey without specifying an enrollment code, we look for the nearest workshop that they have attended, but this doesn’t match the workshop for a pre survey because they haven’t attended yet. For the pre survey, we will need to look up the nearest workshop they have enrolled in, instead.
This fix adds a new function that only looks up the nearest workshop they have enrolled in without regard for attendance.

Testing story

We reproduced the issue locally and validated the fix will show the user the correct survey instead of a 404.

Reviewer Checklist:

  • Tests provide adequate coverage
  • Code is well-commented
  • New features are translatable or updates will not break translations
  • Relevant documentation has been added or updated
  • User impact is well-understood and desirable
  • Pull Request is labeled appropriately
  • Follow-up work items (including potential tech debt) are tracked and linked

@molly-moen molly-moen requested review from breville and a team June 5, 2020 20:41
Workshop.
where(course: [COURSE_CSD, COURSE_CSP]).
where.not(subject: SUBJECT_FIT).
nearest_enrolled_in_by(current_user)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the nearest_attended_or_enrolled_in_by method poorly named? I would have thought that method does what you'd like (probably what you thought as well when you used it :).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's more stringent than named really. First it looks for the nearest workshop with attendance, then if it can't find one it looks for the nearest enrolled one. So a bit of a confusing name, yes

Copy link
Member

Choose a reason for hiding this comment

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

I think such a change is probably out of scope for this bug fix, but maybe nearest_attended_else_enrolled_in_by? :)

Copy link
Contributor

Choose a reason for hiding this comment

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

To be clear I'm talking about the old method, not your new one :) I guess I still don't understand, was the problem that the survey failed for teachers who have attended some other workshop in the past?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah the issue was if a teacher had attended another workshop previously it was trying to load a survey which didn't exist (ex. we don't have a foorm survey for academic year workshops). This was only an issue for the pre-survey because for everything else the teacher should have been marked attended for this workshop.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 got it thanks!

@molly-moen molly-moen merged commit a9bc211 into staging Jun 5, 2020
@molly-moen molly-moen deleted the molly/fix-pre-survey-bug branch June 5, 2020 22:26
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.

None yet

3 participants