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

Protect against buggy participants reports from Aalto SISU API #1181

Merged
merged 1 commit into from
May 17, 2023

Conversation

ihalaij1
Copy link
Contributor

Description

What?

It has turned out that Aalto's SISU API gateway occasionally returns an empty list for all courses queried using this API because of some malfunction. This causes all students to be removed from course enrollments (and a major hassle as a result). Therefore we have now added a check to the code to protect against this situation: if the response contains an empty participant list, it is ignored.

Additionally, the error handling now reports the error code more clearly in logs.

Fixes #1180

Testing

What type of test did you run?

  • Accessibility test using the WAVE extension.
  • Django unit tests.
  • Selenium tests.
  • Other test. (Add a description below)
  • Manual testing.

This fix was not tested, because of the difficulty of doing that, but I assume there are no problems.

Did you test the changes in

  • Chrome
  • Firefox
  • This pull request cannot be tested in the browser.

Programming style

  • Did you follow our style guides?
  • Did you use Python type hinting in all functions that you added or edited? (type hints for function parameters and return values)

Have you updated the README or other relevant documentation?

  • documents inside the doc directory.
  • README.md.
  • Aplus Manual.
  • Other documentation (mention below which documentation).

Is it Done?

  • Reviewer has finished the code review
  • After the review, the developer has made changes accordingly
  • Customer/Teacher has accepted the implementation of the feature

@ihalaij1 ihalaij1 added this to the v1.19 milestone May 17, 2023
@ihalaij1 ihalaij1 force-pushed the fix-sis branch 2 times, most recently from 98bb794 to f9e795d Compare May 17, 2023 08:30
@ihalaij1 ihalaij1 added area: enrollment Related to course enrollments area: student information systems Related to student information systems (SIS), e.g. Oodi, SISU, Salesforce labels May 17, 2023
Copy link
Contributor

@PasiSa PasiSa left a comment

Choose a reason for hiding this comment

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

Couple of change proposal related to logging. What do you think?

course/models.py Outdated Show resolved Hide resolved
course/models.py Show resolved Hide resolved
@ihalaij1
Copy link
Contributor Author

Couple of change proposal related to logging. What do you think?

I made the suggested changes!

Copy link
Contributor

@PasiSa PasiSa left a comment

Choose a reason for hiding this comment

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

Thanks, looks good to me.

Copy link
Contributor

@markkuriekkinen markkuriekkinen left a comment

Choose a reason for hiding this comment

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

Well done! I have one preference, though. I like to include a bit more background in the git commit message. If there is only a link to the issue, then you have to open the issue to remember what the problem really was.

I will merge this now.

@markkuriekkinen markkuriekkinen merged commit fd87748 into apluslms:master May 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: enrollment Related to course enrollments area: student information systems Related to student information systems (SIS), e.g. Oodi, SISU, Salesforce
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Protect against buggy participants reports from Aalto SISU API
3 participants