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

Create provider wkpb.de #267

Merged
merged 3 commits into from
Feb 12, 2024
Merged

Create provider wkpb.de #267

merged 3 commits into from
Feb 12, 2024

Conversation

JulianGro
Copy link
Contributor

This adds configuration for school internal email accounts.
Since students currently end up with 4 different accounts, I added a comment to remind them which account this uses.

Copy link
Contributor

@r10s r10s left a comment

Choose a reason for hiding this comment

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

thank you for your PR!

i assume, you want to display the text "Dies sind die gleichen Anmeldedaten wie bei Moodle und Abitur-Online." when the user focuses the password field? sth. as:

in this case, you need to set the status to PREPARATION, otherwise, before_login_hint is not displayed.

also, "Mehr Informationen" links to the line "Student and teacher email of the Westfalen-Kolleg in Paderborn." in then, if you think,this is not meaningful enough, you can also add more information there

@@ -0,0 +1,21 @@
---
name: Westfalen-Kolleg Paderborn
status: OK
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
status: OK
status: PREPARATION

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm. It works out of the box with this, so technically doesn't need "preparation".
What do you think about making the before_login_hint also show with the OK status in the future? In this specific case, it makes perfect sense to me to do so. I don't see any harm in changing it as well, as long as we make sure no files have some old …login_hint with their OK status.

Whatever the case, this hint isn't particularly important; It is just useful. I would just keep it the way it is right now.

Copy link
Contributor

@r10s r10s Dec 3, 2023

Choose a reason for hiding this comment

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

yes, considering to show before_login_hint also for status OK makes sense. it was just not a use case up to now, but in your case, i get the point.

however, redefining API would required adaption in all UI and is a longer process, taking time.

therefore, pragmatic approach for now, it seems easier to go for PREPARATION (the only disadvantage i see with that is that https://providers.delta.chat will show that state as well), in the apps, things should be just fine.
maybe add a comment to the file that PREPARATION is only needed to get before_login_hint displayed.

if it turns out that at least one or two other providers need such an refinement, we can alter the API - and change your status back to OK

Copy link
Contributor

Choose a reason for hiding this comment

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

or just merge it as is and no hint will be shown until this is corrected, in fact checking if there is a hint and displaying it makes more sense than needing to set preparation needed

Copy link
Contributor

@r10s r10s Jan 18, 2024

Choose a reason for hiding this comment

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

yes, we can also do it that way, but that'll take time. @adbenitez can you adapt android and maybe core (or file an issue there)?

but we can still use PREPARATION for now to fix the issue immediately on all platform

@r10s
Copy link
Contributor

r10s commented Dec 3, 2023

another thing: the file should be renamed to wkpb.de.md

@adbenitez
Copy link
Contributor

another thing: the file should be renamed to wkpb.de.md

I renamed it

@r10s
Copy link
Contributor

r10s commented Jan 18, 2024

even though semantically not 100% correct, i changed this pr to PREPARATION so that the school has immediate benefit on the next update. afaik, there are no other side-effects

we can change that later to OK when platforms are adapted (i think, this also requires core changes, and core is quite busy with other things currently)

@missytake missytake merged commit cc8d805 into chatmail:master Feb 12, 2024
2 checks passed
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.

4 participants