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

Improve firm ("other org") sign-up workflow #3595

Merged

Conversation

cmsetzer
Copy link
Contributor

@cmsetzer cmsetzer commented Sep 3, 2024

This is a second attempt at #3593, which I accidentally closed (oops). Included are the following changes to the firm sign-up process:

  • Removes some use cases (journals, faculty) from the Sign Up section and retitles the Law Firms section as "Other Orgs."
  • Updates the Sign Up for Free section on the Perma landing page to reflect the above changes.
  • Updates various copy text and links to reflect the above changes.
  • Modifies the sign-up form on the Other Orgs (formerly Law Firms) section to include more detailed information, and includes that information in the email it generates.
  • Updates tests to match the updated form.
    • Note: I attempted a fresh rewrite of all the "new firm" tests but once I started pulling on that thread, things quickly ballooned out of scope. What's here is a more modest edit of the existing tests.

To validate

Run Perma locally from this branch and inspect the updated sections:

You can validate the form-generated email by submitting the form and then checking your terminal output for the rendered email body.

@cmsetzer cmsetzer requested a review from a team as a code owner September 3, 2024 16:41
Copy link

codecov bot commented Sep 3, 2024

Codecov Report

Attention: Patch coverage is 93.75000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 68.93%. Comparing base (0ba942a) to head (eecb83f).
Report is 21 commits behind head on develop.

Files with missing lines Patch % Lines
perma_web/perma/views/user_management.py 90.00% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #3595      +/-   ##
===========================================
+ Coverage    68.81%   68.93%   +0.11%     
===========================================
  Files           48       48              
  Lines         7014     7030      +16     
===========================================
+ Hits          4827     4846      +19     
+ Misses        2187     2184       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rebeccacremona
Copy link
Contributor

Whoops, I think the CSS commit accidentally got the bundle instead of the source.

I'm assuming there was supposed to be a

.col-sm-by4 {
  width: 100%;
  @include respond-desktop {
    width: 100 / 4 * 1%;
    float: left;
  }
}

in style-responsive.css 🙂 .

Copy link
Contributor

@rebeccacremona rebeccacremona left a comment

Choose a reason for hiding this comment

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

This is such an improvement. Huzzah!

Chris I feel like the preparation of this PR bordered on a hazing-level experience; my gratitude and thanks and embarrassed apologies that it was such a beast!

I noticed a couple tiny omissions (missing css; template variable name change; template text tweak), but everything substantive looks great.

I also wanted to throw one thing out there. The checkbox for "Would you be an administrator on this account?" seems a little odd to me.

image

I think most of the time, checkboxes are labeled with statements, not with questions.

I think it would be more natural for this to be a set of radio buttons, "yes" and "no", possibly with "no" selected by default (to match the behavior of the checkbox).

What would you think of that? Or do you think that's a question for Clare?

perma_web/perma/templates/email/admin/firm_request.txt Outdated Show resolved Hide resolved
perma_web/perma/views/user_management.py Show resolved Hide resolved
@cmsetzer
Copy link
Contributor Author

cmsetzer commented Sep 5, 2024

Thanks for the sanity check and spotting my oversights — all should be fixed now.

I agree about the semantics of the "Would you be an admin?" toggle, and have switched it from a checkbox to a dropdown as we discussed in another thread.

Let me know if this looks good. I'll submit another PR to remove the dead code for journals/faculty.

@rebeccacremona
Copy link
Contributor

🤘

@cmsetzer cmsetzer merged commit 469d69e into harvard-lil:develop Sep 5, 2024
2 checks passed
@cmsetzer cmsetzer deleted the remove-some-use-cases-from-sign-up-page branch September 5, 2024 23:30
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.

2 participants