search

mawoka-myblock / ClassQuiz

ClassQuiz is a quiz-application like Kahoot!, but open-source.
https://classquiz.de
Mozilla Public License 2.0
440 stars 80 forks source link

Update Ormar and Pydantic #384

Closed BogPlaymate closed 2 months ago

BogPlaymate commented 3 months ago

:hearts: Thank you!

mawoka-myblock commented 3 months ago

Okay, I've written down some feedback and notes to ourselves to check some things throughout everything and forgive my impolite language in those comments

BogPlaymate commented 3 months ago

Work is a lot easier one at a time.

On Mon, Jul 8, 2024 at 7:07 AM Marlon @.***> wrote:

@.**** commented on this pull request.

In frontend/src/lib/components/commandpalette.svelte https://github.com/mawoka-myblock/ClassQuiz/pull/384#discussion_r1667773400 :

@@ -242,7 +247,7 @@ SPDX-License-Identifier: MPL-2.0

<input type="text"

  • autofocus
  • comment="svelte recommends against autofocus for accessibility reasons. Therefore removed autofocus command for this input"

Sounds awesome, but let's try to keep things with one at a time and for now, let's focus on getting Pydantic V2 and the new ormar working.

— Reply to this email directly, view it on GitHub https://github.com/mawoka-myblock/ClassQuiz/pull/384#discussion_r1667773400, or unsubscribe https://github.com/notifications/unsubscribe-auth/BAUGNEBAAHRGTQWM23EKNMLZLG327AVCNFSM6AAAAABKM7JF3GVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCNRRHEZTCMZQGU . You are receiving this because you authored the thread.Message ID: @.***>

BogPlaymate commented 2 months ago

I'm looking into it. I will test it and get back to you in ten minutes.

On Mon, Jul 8, 2024 at 6:45 PM Marlon @.***> wrote:

@.**** commented on this pull request.

In classquiz/routers/quiz.py https://github.com/mawoka-myblock/ClassQuiz/pull/384#discussion_r1668327564 :

@@ -56,7 +56,8 @@ class PublicQuizResponseUser(BaseModel):

class PublicQuizResponse(Quiz.get_pydantic()): user_id: PublicQuizResponseUser

  • questions: list[QuizQuestion]

  • questions: list[QuizQuestion]

  • var_questions: list[QuizQuestion] = Field(..., alias="questions")

Last obvious problem before merge

— Reply to this email directly, view it on GitHub https://github.com/mawoka-myblock/ClassQuiz/pull/384#discussion_r1668327564, or unsubscribe https://github.com/notifications/unsubscribe-auth/BAUGNEGXHP5OPOWP7ZF62F3ZLJNSLAVCNFSM6AAAAABKM7JF3GVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCNRSHAYTEOJVHA . You are receiving this because you authored the thread.Message ID: @.***>

BogPlaymate commented 2 months ago

Hmm. If I run the origin code, I get:

"/home/joe/.local/lib/python3.10/site-packages/pydantic/_internal/_fields.py:200: UserWarning: Field name "questions" in "PublicQuizResponse" shadows an attribute in parent "Quiz_ISL" warnings.warn(" So...something to do with pydantic. Not sure if the warning can be safely ignored. I'll keep researching.

On Mon, Jul 8, 2024 at 7:12 PM Joe Fox @.***> wrote:

I'm looking into it. I will test it and get back to you in ten minutes.

On Mon, Jul 8, 2024 at 6:45 PM Marlon @.***> wrote:

@.**** commented on this pull request.

In classquiz/routers/quiz.py https://github.com/mawoka-myblock/ClassQuiz/pull/384#discussion_r1668327564 :

@@ -56,7 +56,8 @@ class PublicQuizResponseUser(BaseModel):

class PublicQuizResponse(Quiz.get_pydantic()): user_id: PublicQuizResponseUser

  • questions: list[QuizQuestion]

  • questions: list[QuizQuestion]

  • var_questions: list[QuizQuestion] = Field(..., alias="questions")

Last obvious problem before merge

— Reply to this email directly, view it on GitHub https://github.com/mawoka-myblock/ClassQuiz/pull/384#discussion_r1668327564, or unsubscribe https://github.com/notifications/unsubscribe-auth/BAUGNEGXHP5OPOWP7ZF62F3ZLJNSLAVCNFSM6AAAAABKM7JF3GVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCNRSHAYTEOJVHA . You are receiving this because you authored the thread.Message ID: @.***>

mawoka-myblock commented 2 months ago

Man, it's just a warning, not an error, so nothing to worry about ;) But feel free to investigate further. That's just my opinion

BogPlaymate commented 2 months ago

You're probably right, it's not important. However, what is Quiz_ISL? It doesn't seem to be in the code.

On Mon, Jul 8, 2024 at 7:19 PM Marlon @.***> wrote:

Man, it's just a warning, not an error, so nothing to worry about ;) But feel free to investigate further. That's just my opinion

— Reply to this email directly, view it on GitHub https://github.com/mawoka-myblock/ClassQuiz/pull/384#issuecomment-2213618559, or unsubscribe https://github.com/notifications/unsubscribe-auth/BAUGNEDP5YYF7FTEMUTEL5DZLJRUVAVCNFSM6AAAAABKM7JF3GVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMJTGYYTQNJVHE . You are receiving this because you authored the thread.Message ID: @.***>

BogPlaymate commented 2 months ago

We should resolve the naming conflict.

"Quiz_ISL" appears to be a class generated by pedantic (or possibly one of our other helper libraries).

This class is derived from Quiz, a class described in ClassQuiz/classquiz/db line 170. It has a property: questions: Json[list[QuizQuestion]] = ormar.JSON(nullable=False) Class PublicQuizResponse, in ClassQuiz/classquiz/routers line 57 inherits from Quiz. It has a different property with the same name: questions: list[QuizQuestion].

Which of these properties shall we rename? (Or, perhaps we can delete one...)

If you are able to choose a new name, then I will go through the code and implement the change. Even if we choose randomly which to rename, that is okay.

On Mon, Jul 8, 2024 at 7:34 PM Joe Fox @.***> wrote:

You're probably right, it's not important. However, what is Quiz_ISL? It doesn't seem to be in the code.

On Mon, Jul 8, 2024 at 7:19 PM Marlon @.***> wrote:

Man, it's just a warning, not an error, so nothing to worry about ;) But feel free to investigate further. That's just my opinion

— Reply to this email directly, view it on GitHub https://github.com/mawoka-myblock/ClassQuiz/pull/384#issuecomment-2213618559, or unsubscribe https://github.com/notifications/unsubscribe-auth/BAUGNEDP5YYF7FTEMUTEL5DZLJRUVAVCNFSM6AAAAABKM7JF3GVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMJTGYYTQNJVHE . You are receiving this because you authored the thread.Message ID: @.***>

mawoka-myblock commented 2 months ago

If we talk renaming, then definitely the second PublicQuizResponse as this should be pretty much a single-use class, just to structure a response. The other one represents the db.

BogPlaymate commented 2 months ago

Right! Yes, it seems safe to rename the questions variable in PublicQuizResponse.

There is a way of overriding attributes in python too, which would not require a name change. However, that requires creating custom setters, getters, and deleters, so it would take up a lot more code.

I've pushed this minor name change to the fork.

On Tue, Jul 9, 2024 at 1:58 AM Marlon @.***> wrote:

If we talk renaming, then definitely the second PublicQuizResponse as this should be pretty much a single-use class, just to structure a response. The other one represents the db.

— Reply to this email directly, view it on GitHub https://github.com/mawoka-myblock/ClassQuiz/pull/384#issuecomment-2214700584, or unsubscribe https://github.com/notifications/unsubscribe-auth/BAUGNEFJTVYUWWYWQV3QMZLZLLALVAVCNFSM6AAAAABKM7JF3GVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMJUG4YDANJYGQ . You are receiving this because you authored the thread.Message ID: @.***>

mawoka-myblock commented 2 months ago

Thank you soo much for your effort! I'll do a final review soon.

BogPlaymate commented 2 months ago

Great! Take your time.

I just had to push it again, because I forgot to empty the mail_server, mail_port etc fields after testing. The version there now should be okay for the main branch.

I am glad to finally start being useful.

Joe

On Tue, Jul 9, 2024 at 6:01 PM Marlon @.***> wrote:

Thank you soo much for your effort! I'll do a final review soon.

— Reply to this email directly, view it on GitHub https://github.com/mawoka-myblock/ClassQuiz/pull/384#issuecomment-2217045985, or unsubscribe https://github.com/notifications/unsubscribe-auth/BAUGNED26R5T673GOSHMGHDZLORHDAVCNFSM6AAAAABKM7JF3GVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMJXGA2DKOJYGU . You are receiving this because you authored the thread.Message ID: @.***>

mawoka-myblock commented 2 months ago

Please describe your changes on the commits directly, not in their description, but feel free to provide additional details there. Maybe also try to create separate commits for different changes. Just a tip for next time, but I still admire the effort you put in the commit descriptions.

BogPlaymate commented 2 months ago

Ah, okay. "Describe directly" means I should scroll down when using git commit?

I will break up the commits into single issue actions starting next time.

On Wed, Jul 10, 2024, 07:11 Marlon @.***> wrote:

Please describe your changes on the commits directly, not in their description, but feel free to provide additional details there. Maybe also try to create separate commits for different changes. Just a tip for next time, but I still admire the effort you put in the commit descriptions.

— Reply to this email directly, view it on GitHub https://github.com/mawoka-myblock/ClassQuiz/pull/384#issuecomment-2218816909, or unsubscribe https://github.com/notifications/unsubscribe-auth/BAUGNEDEEDWKZUQSZJDXMRTZLRNXJAVCNFSM6AAAAABKM7JF3GVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMJYHAYTMOJQHE . You are receiving this because you authored the thread.Message ID: @.***>

mawoka-myblock commented 2 months ago

A lot of the other copy files are still there, see the files tab.

BogPlaymate commented 2 months ago

Thanks for the tip about VS Code(ium).

I think I've deleted the unused files now.

mawoka-myblock commented 2 months ago

Yes, looks good! Using git via the command line is something I've started doing ~3 years after starting to use git for every project, so please don't feel bad! Git is also such a complex and powerful piece of software that I definitely have only consciously used the tip of the iceberg.

BogPlaymate commented 2 months ago

:) Git is good, huh?

Let me know if there's other useful things which I can do.

On Thu, Jul 11, 2024 at 6:37 AM Marlon @.***> wrote:

Yes, looks good! Using git via the command line is something I've started doing ~3 years after starting to use git for every project, so please don't feel bad! Git is also such a complex and powerful piece of software that I definitely have only consciously used the tip of the iceberg.

— Reply to this email directly, view it on GitHub https://github.com/mawoka-myblock/ClassQuiz/pull/384#issuecomment-2221557754, or unsubscribe https://github.com/notifications/unsubscribe-auth/BAUGNEBIHDRKE5NT2HL2RM3ZLWSSJAVCNFSM6AAAAABKM7JF3GVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMRRGU2TONZVGQ . You are receiving this because you authored the thread.Message ID: @.***>

mawoka-myblock commented 2 months ago

Atm, I'm fixing some small things in this PR, to get it ready for merge. And please do not force push again, use rebase or merge if possible ;)

BogPlaymate commented 2 months ago

Great, this is the kind of feedback I'm looking for. I'll look into those commands.

Let me know if you have more questions about my code.

On Thu, Jul 11, 2024 at 6:48 AM Marlon @.***> wrote:

Atm, I'm fixing some small things in this PR, to get it ready for merge. And please do not force push again, use rebase or merge if possible ;)

— Reply to this email directly, view it on GitHub https://github.com/mawoka-myblock/ClassQuiz/pull/384#issuecomment-2221587923, or unsubscribe https://github.com/notifications/unsubscribe-auth/BAUGNEESLK23KSXY5S5JL7DZLWT23AVCNFSM6AAAAABKM7JF3GVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMRRGU4DOOJSGM . You are receiving this because you authored the thread.Message ID: @.***>

mawoka-myblock commented 2 months ago

Okay, updating Pydantic is way more involved than we thought. You also haven't really tested anything, have you?

BogPlaymate commented 2 months ago

Sorry, I only ran the run_tests.sh

On Thu, Jul 11, 2024 at 7:12 AM Marlon @.***> wrote:

Okay, updating Pydantic is way more involved than we thought. You also haven't really tested anything, have you?

— Reply to this email directly, view it on GitHub https://github.com/mawoka-myblock/ClassQuiz/pull/384#issuecomment-2221615949, or unsubscribe https://github.com/notifications/unsubscribe-auth/BAUGNEF2IHTN5JT3MX3FSMDZLWWWVAVCNFSM6AAAAABKM7JF3GVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMRRGYYTKOJUHE . You are receiving this because you authored the thread.Message ID: @.***>

mawoka-myblock commented 2 months ago

Okay, I've now tested the most basic functionality and everything should work again, but we/I'll have to test further. I'm gonna throw a quick test ClassQuiz instance together, to, well, test this.

BogPlaymate commented 2 months ago

Yay! Good job!

On Thu, Jul 11, 2024 at 7:29 AM Marlon @.***> wrote:

Okay, I've now tested the most basic functionality and everything should work again, but we/I'll have to test further. I'm gonna throw a quick test ClassQuiz instance together, to, well, test this.

— Reply to this email directly, view it on GitHub https://github.com/mawoka-myblock/ClassQuiz/pull/384#issuecomment-2221634038, or unsubscribe https://github.com/notifications/unsubscribe-auth/BAUGNEFRV3M5EGHFCXCI62DZLWYUXAVCNFSM6AAAAABKM7JF3GVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMRRGYZTIMBTHA . You are receiving this because you authored the thread.Message ID: @.***>

BogPlaymate commented 2 months ago

I have to go to work soon, but I will test things when I get back. So, feel free to leave a note to me for anything that should be investigated.

On Thu, Jul 11, 2024 at 7:30 AM Joe Fox @.***> wrote:

Yay! Good job!

On Thu, Jul 11, 2024 at 7:29 AM Marlon @.***> wrote:

Okay, I've now tested the most basic functionality and everything should work again, but we/I'll have to test further. I'm gonna throw a quick test ClassQuiz instance together, to, well, test this.

— Reply to this email directly, view it on GitHub https://github.com/mawoka-myblock/ClassQuiz/pull/384#issuecomment-2221634038, or unsubscribe https://github.com/notifications/unsubscribe-auth/BAUGNEFRV3M5EGHFCXCI62DZLWYUXAVCNFSM6AAAAABKM7JF3GVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMRRGYZTIMBTHA . You are receiving this because you authored the thread.Message ID: @.***>

mawoka-myblock commented 2 months ago

https://classquiz.test.mawoka.eu/

mawoka-myblock commented 2 months ago

Thank you soo much for your effort and all the time you've already put into understanding and getting into the project! You're awesome!

BogPlaymate commented 2 months ago

🥰 We're building trust

On Thu, Jul 11, 2024, 08:00 Marlon @.***> wrote:

Thank you soo much for your effort and all the time you've already put into understanding and getting into the project! You're awesome!

— Reply to this email directly, view it on GitHub https://github.com/mawoka-myblock/ClassQuiz/pull/384#issuecomment-2221665154, or unsubscribe https://github.com/notifications/unsubscribe-auth/BAUGNEDJW7X7PE6MKI5AJHTZLW4JZAVCNFSM6AAAAABKM7JF3GVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMRRGY3DKMJVGQ . You are receiving this because you authored the thread.Message ID: @.***>

mawoka-myblock commented 2 months ago

You should now also have received a mail for GlitchTip. You should be able to join the "team"

BogPlaymate commented 2 months ago

I want to officially join the team!

Thank you, yes I received an email from GlitchTip.

Unfortunately, when I click it and log in, I still can't see anything. Most pages are blank.

But please do not worry about this now. I hope you have a great sleep. You have earned it.

On Thu, Jul 11, 2024, 08:09 Marlon @.***> wrote:

You should now also have received a mail for GlitchTip. You should be able to join the "team"

— Reply to this email directly, view it on GitHub https://github.com/mawoka-myblock/ClassQuiz/pull/384#issuecomment-2221672665, or unsubscribe https://github.com/notifications/unsubscribe-auth/BAUGNEGHFMI5PBGNPTKS3ALZLW5IZAVCNFSM6AAAAABKM7JF3GVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMRRGY3TENRWGU . You are receiving this because you authored the thread.Message ID: @.***>