Skip to content

Condition of UniqueTogetherValidator can be read-only #9764

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

Merged
merged 2 commits into from
Aug 13, 2025

Conversation

ticosax
Copy link
Contributor

@ticosax ticosax commented Aug 12, 2025

Description

We can't always expect to find the value of the unique condition in the serializer if the field is read-only.

The proposed solution is to fallback on instance's value.

We can't always expect to find the value of the condition in the serializer
if the field is read-only.
@ticosax ticosax changed the title Condition of UniqueValidator can be read-only Condition of UniqueTogetherValidator can be read-only Aug 12, 2025
@auvipy auvipy self-requested a review August 12, 2025 15:26
@ticosax
Copy link
Contributor Author

ticosax commented Aug 12, 2025

Looks like, it would fix #9756
(github was degraded at time of submission, I couldn't access to the list of issues)

@auvipy auvipy requested a review from Copilot August 12, 2025 15:50
Copilot

This comment was marked as resolved.

@ticosax ticosax force-pushed the unique-validator-read-only-field branch from 7528b11 to 9430c49 Compare August 12, 2025 16:36
@auvipy auvipy requested a review from a team August 12, 2025 17:27
@auvipy auvipy merged commit 513ddb4 into encode:master Aug 13, 2025
7 checks passed
@auvipy auvipy added the Bug label Aug 13, 2025
@ticosax ticosax deleted the unique-validator-read-only-field branch August 13, 2025 07:30
@browniebroke
Copy link
Member

Hey @auvipy, that was a bit (too) fast, wasn't it? This PR was merged very quickly compared to our usual turnaround and I feel like it didn't give a chance to other maintainers to review it properly. I want us to be really cautious about changing things, espacially around validation where we've caused a few regressions in the not so distant past... I don't mind merging a PR quickly for trivial things (like a typo) but in this case I would have liked us to wait for another approval.

PS: I think I'm ok about the code in this specific instance, but that's more about the process

@auvipy
Copy link
Member

auvipy commented Aug 13, 2025

yeah, I was feeling like that, just after merging it. actually I did it after getting, the confirmation from the issue comment. but from the next time I will wait atleast 72 hours or more for sufficient time to get multiple review. sorry for this time and thanks for raising it explicitly.

@peterthomassen
Copy link
Collaborator

I'm also OK with the code in this PR, but second the process concerns.

I think a 72-hour deadline is no appropriate, given holidays and other things. When requesting a review from other team members, the best course of action would be to wait for an actual review and approval from the team.

In my view, we should usually have 4 eyes from the team review before merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants