Refactor core/home app views and serializers #663

sgfost · 2023-08-02T21:56:07Z

merges related user/profile serializers to standardize on one minimal profile data type
merges two vue components that both implemented a multiselect search over profiles
also sneaks in a noscript tag in the base template for warning visitors with js disabled
moves views/serializers for memberprofile, events, jobs, etc. so they are all in the same app as their model - comses/planning#136

resolves noscript javascript detection isn't working #490
resolves comses/planning#137

sgfost · 2023-08-02T22:03:41Z

The intent here was to also do some re-organizing of django apps for comses/planning#136, though I'd like to be more confident in where things should end up before hacking away it it, so those changes are left out for now.

Current thought is to move everything MemberProfile to core and events/jobs/etc to home. The member profile serializer's dependency on library for related codebases was removed here already since that was unused and should probably be retrieved differently if it were to be needed.

alee · 2023-08-03T21:00:33Z

does it make more sense to move the events/jobs view functions to core instead? It's not clear they need to live in home (though I guess we should first define what should live in core vs home vs library etc)

sgfost · 2023-08-03T21:41:20Z

My thought with moving events/jobs/etc. to home was to separate them from core functionality that can be used by all other apps so that its "beside" library. Maybe a new app that encapsulates ''non-library content" and use home only for pages and such?

alee · 2023-08-03T21:47:05Z

That's a fair assessment, so home is wagtail related models + basic content related things and core is the shared common-to-all things models + views?

sgfost · 2023-08-03T22:00:50Z

Yep, only issue would be that it adds a few more models/views to the already somewhat large home app but we can always think about a split later on

sgfost · 2023-08-04T21:50:27Z

I'm actually going to walk back wanting to move events and jobs to home, creating and running safe migrations for moving models between apps is a bit of a nightmare and in this case would just net more confusion/potential for problems. There is at least one active PR to django for fixing this so I'd say wait until that makes it into a release to mess around with moving model tables

https://code.djangoproject.com/ticket/24686

Still leaves re-aligning views and serializers though, which is the majority of the refactor anyways

sgfost · 2023-08-08T00:56:20Z

django/core/views.py

+            Codebase.objects.accessible(accessing_user)
+            .filter_by_contributor(instance.user)
+            .with_tags()
+            .with_featured_images()


This is the only bit of weirdness left (importing from library in core to add codebases to memberprofile context). Couldn't come up with a clean solution for it but there may be one..

I think this about as best as we can make it for balancing simplicity + understandability

In the future if Django makes it easier to move models across apps it might be better for core.MemberProfile and its associated views / urls / templates to move to home.MemberProfile so that the core doesn't have any dependencies on other apps, while allowing other apps to have side linkages with each other as necessary

using a standardized minimal member profile/user for contributor, peer reviewer, and profile lists (search endpoint) still need to re-organize apps, minimum of: - profile/user -> core - event/job/etc -> home ref comses/planning#137

contributor search remains a separate component due to using a different data type, though the display was improved - fixed an issue with contributor serializer getting tripped up on finding existing contributors

models are kept in place for now and related code was moved to be in the same app as the model it references the only remaining issue is that core.views imports library.models.Codebases in order to add related codebases to the context for MemberProfileViewSet - removed some lingering references to the now-deprecated Institution model - some additional splitting/cleaning up for readability ref comses/planning#136

additionally, do not overwrite contributor affiliation if it is updated with no aff. provided

add comments including some confusion about why self.templates is being initialized in get_template_names

alee

great refactor! Really like the winnowing down of duplicate types both in the frontend and backend serializers. Some minor comments in the notes though I think there might be some bugs in the conditional logic that aren't getting exposed yet.

If possible it would be great to write test(s) that do expose those pathways first to make the build fail and then the fix...

alee · 2023-08-16T01:33:26Z

django/core/urls.py

-        path("make-error/", views.make_error),
-        path("__debug__/", include(debug_toolbar.urls)),
-    ] + urlpatterns
+urlpatterns += get_dev_urls()


I'm pretty sure this will throw an error in prod e.g., [1, 2, 3] + None. Would need to fix get_dev_urls and get_debug_urls to return [] in the else clause

It might be cleaner / easier to see what's happening by keeping the conditional guard where we mutate urlpatterns (e.g., where they were originally) but don't feel strongly about it either way

Thanks for catching that. Conditionals at the bottom is a bit more concise and maybe easier to follow, I'll go with that

django/core/urls.py

alee · 2023-08-16T01:37:44Z

django/library/serializers.py

            contrib_filter = {
                k: validated_data.get(k, "")
                for k in ["given_name", "family_name", "email"]
            }
-            return None, Contributor.objects.filter(**contrib_filter).first()
+            contributor = Contributor.objects.filter(**contrib_filter).first()


does this clause also need to set user to None or is it OK for it to be the result of validated_data.get("user") at the top of the method?

It may work, but you're right, much better to be explicit

sgfost · 2023-08-16T17:20:58Z

If possible it would be great to write test(s) that do expose those pathways first to make the build fail and then the fix...

I'm assuming you're referring to the contributor serializer?

prevent orphaned MemberProfile instances

- include migration for cascade deleting users with memberprofile (faf0921)

sgfost added refactor ui/ux django rest framework labels Aug 2, 2023

sgfost commented Aug 8, 2023

View reviewed changes

sgfost changed the title ~~Refactor user/profile data~~ Refactor core/home app views and serializers Aug 8, 2023

sgfost and others added 6 commits August 8, 2023 13:58

feat: add noscript warning

8d4a1e7

refactor: consolidate related user/profile serializers

4d12edc

using a standardized minimal member profile/user for contributor, peer reviewer, and profile lists (search endpoint) still need to re-organize apps, minimum of: - profile/user -> core - event/job/etc -> home ref comses/planning#137

refactor: combine user/reviewer search, improve styling

8cd0176

contributor search remains a separate component due to using a different data type, though the display was improved - fixed an issue with contributor serializer getting tripped up on finding existing contributors

refactor: clean up get_existing_contributor

077e941

refactor: handle non-existant username in get_existing_contributors

9d94985

additionally, do not overwrite contributor affiliation if it is updated with no aff. provided

sgfost force-pushed the refactor-user-data branch from 8f0aed0 to 9d94985 Compare August 8, 2023 21:11

sgfost marked this pull request as ready for review August 8, 2023 21:13

refactor: use f strings unless there's hairy logic

02c8f1f

add comments including some confusion about why self.templates is being initialized in get_template_names

alee requested changes Aug 16, 2023

View reviewed changes

alee and others added 3 commits August 16, 2023 15:14

fix: cascade User->MemberProfile deletion

faf0921

prevent orphaned MemberProfile instances

fix: prevent core/urls.py from failing outside of dev

1a8185f

test: add coverage for get_existing_contributor matching

f691f65

- include migration for cascade deleting users with memberprofile (faf0921)

alee approved these changes Aug 18, 2023

View reviewed changes

Merge branch 'main' into refactor-user-data

ffd2f41

alee merged commit 8ac93a7 into comses:main Aug 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor core/home app views and serializers #663

Refactor core/home app views and serializers #663

sgfost commented Aug 2, 2023 •

edited

Loading

sgfost commented Aug 2, 2023

alee commented Aug 3, 2023 •

edited

Loading

sgfost commented Aug 3, 2023

alee commented Aug 3, 2023

sgfost commented Aug 3, 2023

sgfost commented Aug 4, 2023

sgfost Aug 8, 2023 •

edited

Loading

alee Aug 17, 2023

alee left a comment

alee Aug 16, 2023 •

edited

Loading

sgfost Aug 16, 2023

alee Aug 16, 2023 •

edited

Loading

sgfost Aug 16, 2023

sgfost commented Aug 16, 2023

Refactor core/home app views and serializers #663

Refactor core/home app views and serializers #663

Conversation

sgfost commented Aug 2, 2023 • edited Loading

sgfost commented Aug 2, 2023

alee commented Aug 3, 2023 • edited Loading

sgfost commented Aug 3, 2023

alee commented Aug 3, 2023

sgfost commented Aug 3, 2023

sgfost commented Aug 4, 2023

sgfost Aug 8, 2023 • edited Loading

Choose a reason for hiding this comment

alee Aug 17, 2023

Choose a reason for hiding this comment

alee left a comment

Choose a reason for hiding this comment

alee Aug 16, 2023 • edited Loading

Choose a reason for hiding this comment

sgfost Aug 16, 2023

Choose a reason for hiding this comment

alee Aug 16, 2023 • edited Loading

Choose a reason for hiding this comment

sgfost Aug 16, 2023

Choose a reason for hiding this comment

sgfost commented Aug 16, 2023

sgfost commented Aug 2, 2023 •

edited

Loading

alee commented Aug 3, 2023 •

edited

Loading

sgfost Aug 8, 2023 •

edited

Loading

alee Aug 16, 2023 •

edited

Loading

alee Aug 16, 2023 •

edited

Loading