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

clean-up-last-modifier-id: Don't delete guest users #750

Closed
wants to merge 1 commit into from

Conversation

reinhardt
Copy link
Contributor

Ref syslabcom/scrum#2372

@reinhardt reinhardt force-pushed the scrum-2372-fix-clean-up-last-modifier branch from ef7a91d to 968802e Compare July 10, 2024 10:23
@reinhardt reinhardt changed the title clean-up-last-modifier-id: Keep going if user can't be deleted clean-up-last-modifier-id: Don't delete guest users Jul 10, 2024
Fixes sqlalchemy.exc.IntegrityError: (raised as a result of Query-invoked autoflush; consider using a session.no_autoflush block if this flush is occurring prematurely)
(psycopg2.errors.ForeignKeyViolation) update or delete on table "account" violates foreign key constraint "session_last_modifier_id_fkey" on table "session"
DETAIL:  Key (id)=(...) is still referenced from table "session".

Ref syslabcom/scrum#2372
@reinhardt reinhardt force-pushed the scrum-2372-fix-clean-up-last-modifier branch from 968802e to 64ca1ac Compare July 10, 2024 11:13
@reinhardt reinhardt requested a review from ale-rt July 10, 2024 11:13
Copy link
Member

@ale-rt ale-rt left a comment

Choose a reason for hiding this comment

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

It seems to me we are removing code that we would like to have just because it is hard to fix it.
It does not feel right.

I have no idea if we are removing a feature which is requested by some customer.

I am wondering if adding a session.flush() or even an intermediate commit after changing the last_modifier_id might help.

@pilz do you know if somebody cares?

@reinhardt , I am available to pair on this to find a solution that keeps the feature.

@reinhardt
Copy link
Contributor Author

I have no idea if we are removing a feature which is requested by some customer.

No. The main point is to fix a database state that makes trouble with the guest user cleanup script (#2155). If we don't delete the users here, they will be deleted there: https://github.com/euphorie/osha.oira/blob/main/src/osha/oira/sql_scripts.py#L55

FTR OSHA has some concern about guest users created by bots, but this is about guest sessions that were taken over by registered users and therefore cannot have been created by bots.

I am wondering if adding a session.flush() or even an intermediate commit after changing the last_modifier_id might help.

I had no succes with that, as mentioned in https://github.com/syslabcom/scrum/issues/2372#issuecomment-2180401673

I have doubts whether it's worth spending more time on this. What do you think @pilz? (So far it's about half a day.)

@ale-rt
Copy link
Member

ale-rt commented Jul 12, 2024

I have doubts whether it's worth spending more time on this. What do you think @pilz? (So far it's about half a day.)

I came up with #751

IMO the knowledge acquired it is worth the amount of time spent.

Copy link
Member

@ale-rt ale-rt left a comment

Choose a reason for hiding this comment

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

I am pretty sure this will never do anything because of plone protect. In #751 I had to add an explicit commit to make it pass the csrf protection.

@reinhardt
Copy link
Contributor Author

Does plone.protect also protect postgresql?

In any case, closing this in favour of #751.

@ale-rt ale-rt closed this Jul 31, 2024
@ale-rt ale-rt deleted the scrum-2372-fix-clean-up-last-modifier branch July 31, 2024 08:48
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.

2 participants