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

#2490 - SPIKE: Research students unable to submit updates/content to their profiles #933

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

patrickcuagan
Copy link
Collaborator

Ticket: https://projects.torchbox.com/projects/rca-django-cms-project/tickets/2490

This PR fixes an issue where the school field does not exist when a student edits their own page.

Issue: We're not rendering the form fields if a student is not logged in. The formset doesn't see those fields so an error is raised.

@codecov-commenter
Copy link

codecov-commenter commented Mar 3, 2023

Codecov Report

Merging #933 (53e2852) into master (8775d4d) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #933   +/-   ##
=======================================
  Coverage   52.53%   52.53%           
=======================================
  Files         160      160           
  Lines        7522     7522           
  Branches      164      164           
=======================================
  Hits         3952     3952           
  Misses       3453     3453           
  Partials      117      117           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Collaborator

@kevinhowbrook kevinhowbrook left a comment

Choose a reason for hiding this comment

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

Thanks @patrickcuagan, if there is any time left one improvement here would be creating some more tests to visually check the presence of these fields (in case the templates are changed further)

@patrickcuagan
Copy link
Collaborator Author

Thanks @patrickcuagan, if there is any time left one improvement here would be creating some more tests to visually check the presence of these fields (in case the templates are changed further)

Thanks Kev - I've added a test now.

@patrickcuagan patrickcuagan force-pushed the bugfix/2490-students-unable-to-submit branch from a558322 to 53e2852 Compare March 8, 2023 03:07
GroupPagePermission.objects.create(
group=self.student_group,
page=self.student_index,
permission_type="edit",
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @patrickcuagan I needed to revert this work as it was pushed to dev and causing problem with deploying the Wagtail 5.1 upgrade to dev for testing.

My suggestion here is untested but reflects the changes required here: https://docs.wagtail.org/en/stable/releases/5.1.html#grouppagepermission-now-uses-django-s-permission-model

Suggested change
permission_type="edit",
permission=Permission.objects.get(
content_type__app_label="wagtailcore", codename="change_page"
)

It may be best to wait until the 5.1 upgrade has been deployed and the rebase your work to see the error in the test.

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.

4 participants