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

Archive Item Titles #3345

Merged
merged 31 commits into from
May 29, 2024
Merged

Archive Item Titles #3345

merged 31 commits into from
May 29, 2024

Conversation

chrisvanrun
Copy link
Contributor

Part of the pitch:

This is a copy of what was done for Display Items. I've opted to abstract the CIV set creation and update form data processing into CIVSetCreateViewMixin and CIVSetCreaViewMixin respectively as to stay DRY.

@chrisvanrun chrisvanrun requested review from amickan and jmsmkn as code owners May 7, 2024 10:00
@chrisvanrun
Copy link
Contributor Author

The Django test seemed to fail randomly on not being able to find the PrefixConverter. I assume it has to do with dependency differences on main vs this branch.

Merging main and re-running.

@chrisvanrun
Copy link
Contributor Author

The Django test seemed to fail randomly on not being able to find the PrefixConverter. I assume it has to do with dependency differences on main vs this branch.

Merging main and re-running.

Well, Chris of the past, that worked!

@chrisvanrun chrisvanrun requested a review from amickan May 13, 2024 12:20
@chrisvanrun
Copy link
Contributor Author

Draft, since I am in the middle of a refactor.

@chrisvanrun chrisvanrun marked this pull request as ready for review May 14, 2024 12:36
@chrisvanrun
Copy link
Contributor Author

Heavy refactor. I've now pushed the processing of the data into the forms, leaning more heavily on the model's definition of civ_set. I think this is the DRY-est it has been.

For instance, the forms for DisplaySet creating and updating are now an interesting mix of Mixins:

class DisplaySetCreateForm(
    DisplaySetFormMixin,
    UniqueTitleCreateFormMixin,
    CIVSetCreateFormMixin,
    MultipleCIVForm,
):
    pass


class DisplaySetUpdateForm(
    DisplaySetFormMixin,
    UniqueTitleUpdateFormMixin,
    CIVSetUpdateFormMixin,
    MultipleCIVForm,
):
    def __init__(self, *args, **kwargs):
        super().__init__(*args, **kwargs)
        if not self.instance.is_editable:
            for _, field in self.fields.items():
                field.disabled = True

@chrisvanrun chrisvanrun requested a review from jmsmkn May 14, 2024 12:42
@chrisvanrun chrisvanrun requested a review from amickan May 24, 2024 08:44
@chrisvanrun chrisvanrun requested a review from amickan May 24, 2024 12:42
amickan
amickan previously approved these changes May 24, 2024
Copy link
Contributor

@amickan amickan left a comment

Choose a reason for hiding this comment

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

LGTM!

@chrisvanrun chrisvanrun merged commit 43ba70e into main May 29, 2024
8 checks passed
@chrisvanrun chrisvanrun deleted the archive-item-names branch May 29, 2024 06:55
jmsmkn pushed a commit that referenced this pull request May 31, 2024
Fixes bug introduced in
#3345 which made
archive items uneditable.
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.

3 participants