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

Näytetään hakemuksen muokkaajan nimi hakemusnäkymässä (nyt näkyy vain aikaleima) #6083

Merged
merged 1 commit into from
Dec 13, 2024

Conversation

kechpaja-at-gofore
Copy link
Collaborator

Lisätä muokkaajan nimi hakemusnäkymään muokkausajan alle:

2024-12-04-151136_723x342_scrot

Muutin myös create ja updated -kenttien nimet standardien mukaisiksi.

@kechpaja-at-gofore kechpaja-at-gofore added the enhancement Uusi toiminnallisuus tai parannus label Dec 4, 2024
@kechpaja-at-gofore kechpaja-at-gofore force-pushed the show-application-modifier-name branch 3 times, most recently from ae75661 to 04d83f4 Compare December 11, 2024 09:51
@kechpaja-at-gofore kechpaja-at-gofore marked this pull request as ready for review December 11, 2024 10:22
@kechpaja-at-gofore kechpaja-at-gofore requested review from Joosakur, akheron, Gekkio, reynders and Wnt and removed request for Joosakur December 11, 2024 10:22
@kechpaja-at-gofore kechpaja-at-gofore force-pushed the show-application-modifier-name branch from 04d83f4 to d91c482 Compare December 11, 2024 11:01
Comment on lines +1037 to +1038
now: HelsinkiDateTime,
modifiedBy: EvakaUserId,
Copy link
Contributor

Choose a reason for hiding this comment

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

Many of the functions here including this one aren't really called by the user. Instead they are sort of secondary side-effects that result from some primary action such as changing status and only update some metadata-like fields an not really the application itself, even though it is an update to the application table. Now one user request ends up updating these fields multiple times which is at least unnecessary although not really harmful.

One thing to also consider is that if we keep the separate status_modified fields, then should the normal modified_by be changed when only status changes? If not, then these queries which are essentially part of the status update process should probably not update it either.

No right or wrong answers really, just something you could think about if you didn't yet. Otherwise looks good!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Now one user request ends up updating these fields multiple times which is at least unnecessary although not really harmful.

I agree that this might be worth changing at some point, but I don't think it can be fixed without going too far out of scope for this change.

WRT the status_modified fields: I think any changes to them are out of scope for this change as well. I could be convinced that they should be removed, but that would need to be a separate work item, and would require rethinking a number of things. As things are, and especially given the possibility of removing status_modified_*, it makes sense to update modified_at every time.

Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

fair enough

@kechpaja-at-gofore kechpaja-at-gofore force-pushed the show-application-modifier-name branch from d91c482 to f368a22 Compare December 13, 2024 09:17
@kechpaja-at-gofore kechpaja-at-gofore enabled auto-merge (squash) December 13, 2024 09:18
@kechpaja-at-gofore kechpaja-at-gofore merged commit 55757b8 into master Dec 13, 2024
28 checks passed
@kechpaja-at-gofore kechpaja-at-gofore deleted the show-application-modifier-name branch December 13, 2024 09:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Uusi toiminnallisuus tai parannus
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants