Skip to content

Reverts account and frozen author fields to charfields.#4406

Merged
ajrbyers merged 8 commits into
r-v1.7.xfrom
b-4405-bugfix
Sep 18, 2024
Merged

Reverts account and frozen author fields to charfields.#4406
ajrbyers merged 8 commits into
r-v1.7.xfrom
b-4405-bugfix

Conversation

@ajrbyers

@ajrbyers ajrbyers commented Sep 13, 2024

Copy link
Copy Markdown
Member

This PR reverts changes to Account and FrozenAuthor models, returning them to CharFields. There are three reasons for this:

  1. As reported in Registration form defects ahead of v1.7.0 upgrade #4405 when form media is not present these fields display as text-areas (even in material, they just happened to be a single line)
  2. We're not sure that down-stream indexers will accept these fields with HTML in them
  3. Overwriting the widgets to be charfields doesn't work out of the box and the display of large numbers of mini HTML editors is not great

This PR also adds a button_classes variable to orcid_registration.html so that each theme can style the button with the correct classes.

ORCID button after:
Screenshot 2024-09-13 at 11 22 26
Screenshot 2024-09-13 at 11 22 45
Screenshot 2024-09-13 at 11 22 53

@ajrbyers ajrbyers changed the base branch from master to r-v1.7.x September 13, 2024 10:08
@mauromsl mauromsl self-requested a review September 13, 2024 11:08

@mauromsl mauromsl left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks! just one minor comment but passing on to Joe to comment on

<a href="{{ settings.ORCID_URL }}?client_id={{ settings.ORCID_CLIENT_ID }}&response_type=code&scope=/authenticate&redirect_uri={% orcid_redirect_uri 'register' %}" class="button expanded orcid-button">{% trans "Register with ORCiD" %}</a>
<a
href="{{ settings.ORCID_URL }}?client_id={{ settings.ORCID_CLIENT_ID }}&response_type=code&scope=/authenticate&redirect_uri={% orcid_redirect_uri 'register' %}"
class="{{ button_classes }}">

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

should we set orcid-button as the default if nothing is provided?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Added a commit with default.

@mauromsl mauromsl requested a review from joemull September 13, 2024 11:09
@joemull joemull assigned mauromsl and unassigned joemull Sep 13, 2024

@joemull joemull left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'll go ahead and request the changes we discussed offline so I know when it's ready to review again.

@mauromsl mauromsl requested a review from joemull September 17, 2024 18:42
@mauromsl mauromsl added the Priority urgent Urgent priority label Sep 17, 2024
@ajrbyers ajrbyers assigned joemull and unassigned mauromsl Sep 18, 2024

@joemull joemull left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I like using a field validator for this--clean and reproducible.

@ajrbyers ajrbyers merged commit 4d5dec8 into r-v1.7.x Sep 18, 2024
@ajrbyers ajrbyers deleted the b-4405-bugfix branch September 18, 2024 10:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Priority urgent Urgent priority

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Registration form defects ahead of v1.7.0 upgrade

3 participants