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

1791: Create application verifications automatically for verein360 applications #1810

Conversation

ztefanie
Copy link
Member

@ztefanie ztefanie commented Dec 3, 2024

Short description

Application verifications get source set for verein360 applications

Proposed changes

  • adjusted database to have column "automaticSource"
  • refined check for incoming applications

Side effects

None, maybe affecting normal application creation process

Testing

create new application with one or more organizations. Check for correct error messages if isAlreadyVerified differs. Check if one or all are true, the applicationVerifications table column automaticSource is set properly to VEREIN360

Feel free to test the complete feature in this PR: #1811

Resolved issues

Fixes: #1791

@ztefanie ztefanie force-pushed the 1791-create-application-verification-automatically-for-verein360 branch from aac3e94 to ea6a8eb Compare December 3, 2024 10:22
Copy link
Contributor

@f1sh1918 f1sh1918 left a comment

Choose a reason for hiding this comment

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

First of all some general thoughts

I'm just thinking of how we "mark" this application in the overview view table and i'm not sure if we really have to define this "source" attribute on a verification but on an application level once. That would simplify that we don't need to loop through all verifications (Even the verification source values will be equal for a application).
Or do you think we will have the case where we have for one application different verification sources?
I would prefer to just add a "Source" column to applications to keep it simple because one application comes typically from one source.
We could then set "Internal" as default source or sth. similar

Maybe i've overseen sth that makes your implementation needed.

@ztefanie
Copy link
Member Author

ztefanie commented Dec 4, 2024

First of all some general thoughts

I'm just thinking of how we "mark" this application in the overview view table and i'm not sure if we really have to define this "source" attribute on a verification but on an application level once. That would simplify that we don't need to loop through all verifications (Even the verification source values will be equal for a application). Or do you think we will have the case where we have for one application different verification sources? I would prefer to just add a "Source" column to applications to keep it simple because one application comes typically from one source. We could then set "Internal" as default source or sth. similar

Maybe i've overseen sth that makes your implementation needed.

Yes of course I thought about this, too. I decided to do it this way for the following reasons:

  1. The most important reasone for me to do so is single responsibility: It is where it logically belongs to, as the applicaiton-verifications are espeically for this use case. Adding the automaticVerified field to application-verifications ensures that each table and entity has a single, clear purpose. The applications table is responsible for storing general information about an application, while application-verifications focuses on tracking and managing the verification details. By adding automaticVerified to application-verifications, we avoid overloading the applications table with information that doesn't directly pertain to its core responsibility.

  2. It is also on this level in the api-endpoint / the graphQL scheme. We cannot change this anymore, as this is already sent to verein360. I think it would be confusing if we handle this incosistently, and putting it on organization/application-verification level in api, but on application level in the datastructure.

  3. If automaticVerified were placed in the applications table, it would create potential redundancy since the same attribute might need to be derived from or overridden by the values in application-verifications. Keeping the field in application-verifications ensures the data is stored in its relevant context, avoiding the need to synchronize changes between multiple levels of the system. Otherwise we would need to check, if application-verifications are consistent with the "already verified" stamp. So for example what would happen, if the application "aleadyVerified" field is set to true, but the applications-verification mail was sent and clicked "rejected". Sure this should never happen, but we somehow would need to catch this case in the code and I think this is more complex, than checking for all application-verifications of one application.

  4. If we plan in the future to extend this feature, it will be easier if the verification step is linked to the organization, than the whole application. E.g. for the feature: Someone works 5 hours at a pre-verified Sportclub, gets an E-Mail: "please add your second volutarty work". Or if we want to add checks, for which organizations verein360 is allowed to set the "automaticVerified" flag, it would be easier to check and handle, e.g. if they set two organziations, one they are allowed to set the automaticVerified flag and for one they are not, we could easily accept the first organization and send and please-verifiy-this-email for the second organization and handle this gracefully.

Base automatically changed from 1624-add-role-for-verein-360-api-tokens to main December 4, 2024 08:04
@f1sh1918
Copy link
Contributor

f1sh1918 commented Dec 4, 2024

Alright thx for the detailed explanation. :)
I will start to review

Copy link
Contributor

@f1sh1918 f1sh1918 left a comment

Choose a reason for hiding this comment

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

The code looks fine to me but i couldn't test if the external source will be set to "Verein360". I added a comment

Copy link
Contributor

@f1sh1918 f1sh1918 left a comment

Choose a reason for hiding this comment

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

Works fine. Nice 👍

@seluianova seluianova self-assigned this Dec 4, 2024
@seluianova
Copy link
Contributor

Seems to be an issue:

  1. create a normal application that requires confirmation from the organization
  2. confirm it using the link from the email
    --> check db: applicationverifcation.automaticSource is VEREIN360

@ztefanie ztefanie force-pushed the 1791-create-application-verification-automatically-for-verein360 branch from dc284d6 to b04a621 Compare December 4, 2024 11:26
}

fun setApplicationVerificationToVerifiedNow(verificationEntities: List<ApplicationVerificationEntity>) {
fun setApplicationVerificationToPreVerifiedNow(verificationEntities: List<ApplicationVerificationEntity>) {
Copy link
Contributor

@seluianova seluianova Dec 4, 2024

Choose a reason for hiding this comment

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

Suggested change
fun setApplicationVerificationToPreVerifiedNow(verificationEntities: List<ApplicationVerificationEntity>) {
fun setApplicationVerificationToPreVerified(verificationEntities: List<ApplicationVerificationEntity>) {

🙃 just a personal preference to keep the names shorter, not sure if 'now' is an important detail here. feel free to ignore

Copy link
Contributor

@seluianova seluianova 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 now!

@seluianova seluianova removed their assignment Dec 4, 2024
@ztefanie ztefanie merged commit bd289ed into main Dec 4, 2024
2 checks passed
@ztefanie ztefanie deleted the 1791-create-application-verification-automatically-for-verein360 branch December 4, 2024 11:53
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.

create application verification automatically for verein360 requests
3 participants