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

feat: add new schedule api #1281

Merged
merged 10 commits into from
Sep 10, 2024
Merged

feat: add new schedule api #1281

merged 10 commits into from
Sep 10, 2024

Conversation

limwa
Copy link
Member

@limwa limwa commented Jul 22, 2024

Closes #1254.
Closes #1280.

Implements behavior to retrieve a user's schedule from the new schedule API.
For now, it reads the HTML from the schedule page and uses the information to retrieve the schedule data from the API.

Removes the old HTML parsing code.

Review checklist

  • Terms and conditions reflect the current change
  • Contains enough appropriate tests
  • If aimed at production, writes a new summary in whatsnew/whatsnew-pt-PT
  • Properly adds an entry in changelog.md with the change
  • If PR includes UI updates/additions, its description has screenshots
  • Behavior is as expected
  • Clean, well-structured code

@limwa limwa requested a review from a team July 22, 2024 10:43
Copy link

codecov bot commented Jul 22, 2024

Codecov Report

Attention: Patch coverage is 5.63380% with 67 lines in your changes missing coverage. Please review.

Project coverage is 16%. Comparing base (d460c06) to head (8e43723).
Report is 5 commits behind head on develop.

Additional details and impacted files
@@           Coverage Diff           @@
##           develop   #1281   +/-   ##
=======================================
- Coverage       16%     16%   -0%     
=======================================
  Files          233     239    +6     
  Lines         7220    7172   -48     
=======================================
- Hits          1139    1078   -61     
- Misses        6081    6094   +13     

Copy link
Member

@thePeras thePeras left a comment

Choose a reason for hiding this comment

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

Looking into the code, seems very nice!

  1. Can we put the response models inside a specific folder? ex lecture_response
  2. Are we keeping the old api code?

@limwa
Copy link
Member Author

limwa commented Jul 23, 2024

  1. Can we put the response models inside a specific folder? ex lecture_response

The response models are already inside the models folder in the new_api directory, so I don't understand what you mean by a "specific folder". Can you please elaborate on that?

  1. Are we keeping the old api code?

I was under the impression we were, but I'm happy to remove it 😄
Personally, I think it might be too soon to rely on this new API at 100%. We can reconsider this later this year...

@thePeras
Copy link
Member

The response models are already inside the models folder in the new_api directory, so I don't understand what you mean by a "specific folder". Can you please elaborate on that?

Sorry, I miss looked the folder. It's nice this way!

  1. Are we keeping the old api code?

I was under the impression we were, but I'm happy to remove it 😄 Personally, I think it might be too soon to rely on this new API at 100%. We can reconsider this later this year

It seems pretty solid, but am not opposing of keeping

@limwa limwa requested a review from thePeras August 21, 2024 15:55
Copy link
Member

@thePeras thePeras left a comment

Choose a reason for hiding this comment

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

I don't have schedules to test, but it seems good to me.
Nice work and what a pleasure to see so much code deleted!

✅ 🛫

@limwa limwa requested a review from a team September 6, 2024 18:31
Copy link
Collaborator

@DGoiana DGoiana 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! 🚀

@DGoiana DGoiana merged commit 8a518bc into develop Sep 10, 2024
6 checks passed
@DGoiana DGoiana deleted the feature/new-schedule-api branch September 10, 2024 15:47
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.

Migrate to the new schedule API 404 when trying to load Lectures from remote
3 participants