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

Partner social media changes #3148

Merged
merged 22 commits into from
Oct 29, 2022
Merged

Partner social media changes #3148

merged 22 commits into from
Oct 29, 2022

Conversation

Learningstuff98
Copy link
Collaborator

@Learningstuff98 Learningstuff98 commented Sep 17, 2022

Resolves #3032

Description

This PR adds an Instagram field to the media information section for partners along with a "No Social Media Presence" checkbox. The checkbox must be checked if none of the social media fields are filled in.

List any dependencies that are required for this change. (gems, js libraries, etc.)

Rspec, Factorybot

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Visual confirmation and I wrote some tests.

Screenshots

Screenshot (555)

Screenshot (573)

Screenshot (557)

With social media presence:
Screenshot (582)

Without social media presence:
Screenshot (581)

When logged in as org_admin1@example.com:
Screenshot (584)

If anything needs to change, please let me know.

Copy link
Collaborator

@cielf cielf left a comment

Choose a reason for hiding this comment

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

Hey @Learningstuff98 -- Thanks for your work on this! -- I do have some notes.

1/ We're really focusing on trying to make things clear for the partners. The error message feels a little awkward as is. Could you change it to "You must either provide a social media site or indicate that you have no social media presence", please?

Copy link
Collaborator

@cielf cielf left a comment

Choose a reason for hiding this comment

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

2/ Is the condition " If and only if partners are shown media section [see partner forms], at least one field in the media section must be filled in." being met properly? If the partners are not being shown the media section, then they shouldn't be getting the errors.

Copy link
Collaborator

@cielf cielf left a comment

Choose a reason for hiding this comment

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

3/ I'd also like to see the "has social media presence and the checkbox is unchecked" test check each of [website, twitter, facebook, instagram] individually.

Let me know if any of these requests are unclear, please? Thanks - cielf

@Learningstuff98
Copy link
Collaborator Author

@cielf I've been attempting to change the error message, but I'm running into difficulties with getting the attribute name "no social media presence" out of the error message. Would it be possible to have an error message that starts with "No social media presence"? I've tried the following resource but I can't get it to work.

Section 3.7 Configuring Active Model
link

@cielf
Copy link
Collaborator

cielf commented Sep 23, 2022

@Learningstuff98 I'm not an expert in this area, by any means -- my notes are from a business pov more than a technical. That being said, one approach that I've found useful is treating it as a translation problem -- i.e. the text that we want is the english version of the text (that could also have other languages). Is that in line with what you're trying to do?

@cielf
Copy link
Collaborator

cielf commented Sep 23, 2022

I do keep forgetting that error messages that read nicely is one of the the things that ends up being harder in rails G I want to figure this out too -- I'll try to take a look tonight, if not, then probably Monday.

@cielf
Copy link
Collaborator

cielf commented Sep 23, 2022

@Learningstuff98 Have you looked at using errors[:base] -- since it is a validation that goes across several fields?

https://guides.rubyonrails.org/active_record_validations.html#errors-base

@Learningstuff98
Copy link
Collaborator Author

@cielf I'll look at this again today, including the errors[:base] suggestion

@Learningstuff98
Copy link
Collaborator Author

Learningstuff98 commented Sep 28, 2022

I have an update. I tried this approach again but I still wasn't able to get it to work, so I ended up using the errors[:base] approach. Here's a screenshot of the current behavior:

Screenshot (564)

I'm going to be busy all day tomorrow, so I'm hoping to send this change when its ready either Thursday or Friday.

Copy link
Collaborator

@cielf cielf left a comment

Choose a reason for hiding this comment

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

Hey @Learningstuff98
Sorry for the delay -- A couple more comments/questions..
1/ Do you need the "if current_partner.errors.include?(:no_social_media_presence)"? IIRC, it's the only error that should plausibly be getting thrown in this section, and if there were any others, I'd want to see the messages. Am I missing something?
2/ The database.yml change -- that shouldn't be part of this.
3/ The "expect(page).not_to have_text("No social media presence must be accepted")" -- I think you can get rid of that.

@Learningstuff98
Copy link
Collaborator Author

@cielf I accidently changed the PG_PASSWORD value in the database.yml, then changed it back to match what it is in main in the "Added a test for the custom error message" commit. No changes to the database.yml file are being proposed in this PR from what I can see.

@cielf
Copy link
Collaborator

cielf commented Oct 7, 2022

@Learningstuff98

Hrm. Hrm. Hrm. Argh.
I think where we're running into trouble and confusion is that if we use :base, we would normally display it at the top of the form, because it is considered an error across the whole object, right.

Since it's such a monster form (and it is a huge ungainly beast of a form), it would be nice to keep it near where the error is.

Argh. There has to be a way to do this without jumping through quite as many hoops as you are. Because some of the things you are doing (like checking for the actual text of the error message before displaying it) are not how things are done at all (and sorry I missed that one on the first go through).

Either we'll figure it out, or I'll give up on my dream of having a reasonable error message. G

@cielf
Copy link
Collaborator

cielf commented Oct 7, 2022

@Learningstuff98
If we go back to the other method... Is this basically what you were trying? (with appropriate indentation, which doesn't seem to be coming through in this comment)

In en.yml....
en:
activemodel: # or activerecord:
errors:
models:
partner:
attributes:
no_social_media_presence:
format: "%{message}"

and then in your model

validates :no_social_media_presence, acceptance: {message: "You must either provide a social media site or indicate that you have no social media presence"}, if: :verified_and_has_no_social_media?

@Learningstuff98
Copy link
Collaborator Author

@cielf I'll try the en.yml approach again. The way that I've been trying definitely isn't ideal.

@cielf
Copy link
Collaborator

cielf commented Oct 7, 2022

@Learningstuff98 Yeah -- sorry to have led you down that particular path. It was an idea, but it turns out, it wasn't the best idea I've ever had.

@Learningstuff98
Copy link
Collaborator Author

@cielf No worries. I'll try the en.yml approach again and hopefully get back soon with some good results.

Copy link
Collaborator

@cielf cielf left a comment

Choose a reason for hiding this comment

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

Hey @Learningstuff98. Because there have been so many adjustments I brought your branch down to my local for some manual testing, and so that I could look at the aggregate changes instead of one thing at a time. Here's what I've got so far from the testing:

I ran rails db:reset , and then signed in as unverified@pawneepregnancy.com

1/ Edit My Organization, clear out Media Information | Update Information
**** “No Social Media Presence “ should show on this page, and it doesn’t
— then “Submit for Approval”
**** you should not be able to successfully submit for approval with the media information in this state (no fields in media information filled in)
2/ Then I signed in as org_admin1@example.com
— Partner Agencies
— Click on “Review Application” for Pawnee Pregnancy Center
**** Media Information section should show the Instagram and No Social Medial Information fields

Copy link
Collaborator

@cielf cielf left a comment

Choose a reason for hiding this comment

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

Hrm. that's not what I meant. Let me reword it: All the fields that are in the media information 'card', including "No Social Media Presence" , should be shown.

@Learningstuff98
Copy link
Collaborator Author

@cielf Is this what you mean?

With social media and no social media presence is unchecked:
Screenshot (579)

Without social media and no social media presence is checked:
Screenshot (580)

@cielf
Copy link
Collaborator

cielf commented Oct 16, 2022

@Learningstuff98 I'd like to see "No Social Media Presence" treated the same way as other checkboxes on the profile form -- for clarity: with the prompt "No Social Media Presence" and then "Yes" , if checked, and "No" if not checked. See checkboxes such as "Case Management" for the pattern. thanks.

@cielf
Copy link
Collaborator

cielf commented Oct 16, 2022

@Learningstuff98 That looks like it should be right . Please also address "**** you should not be able to successfully submit for approval with the media information in this state (no fields in media information filled in)" from my comments a couple of days ago.

@cielf
Copy link
Collaborator

cielf commented Oct 16, 2022

@Learningstuff98 Also make it so that when the bank views the partner, they see all the media fields [see http://localhost:3000/diaper_bank/partners/1 , signed in as org_admin1@example.com]

@Learningstuff98
Copy link
Collaborator Author

@cielf I believe I addressed everything that you mentioned. Can you pull down these changes and test it locally and tell me if anything needs to change?

@cielf
Copy link
Collaborator

cielf commented Oct 16, 2022

@Learningstuff98 Will do, but probably tomorrow at this point.

@cielf
Copy link
Collaborator

cielf commented Oct 17, 2022

@Learningstuff98 Hi! I'll add separate comments for each thing I find from testing from now on.
1/ The banks view of the partner still does not show the new fields: see http://localhost:3000/diaper_bank/partners/1
The view in question is: app/views/profiles/_show.html.erb

@cielf
Copy link
Collaborator

cielf commented Oct 17, 2022

@Learningstuff98
2/ The partner can submit for approval with an entirely blank media section. They shouldn't be able to.

Copy link
Collaborator

@cielf cielf left a comment

Choose a reason for hiding this comment

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

See comments

@cielf
Copy link
Collaborator

cielf commented Oct 17, 2022

@Learningstuff98 Argh. I don't think I pulled it down again. Morning brain. Will retest.

@cielf
Copy link
Collaborator

cielf commented Oct 17, 2022

@Learningstuff98 #1 passes. #2 doesn't. However, that requirement was not in the issue as described.

Copy link
Collaborator

@cielf cielf left a comment

Choose a reason for hiding this comment

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

I'm good with putting this forward and writing a separate issue for checking it on approval.

@Learningstuff98
Copy link
Collaborator Author

@cielf I'm willing to work on said new issue.

@cielf
Copy link
Collaborator

cielf commented Oct 26, 2022

@Learningstuff98 Have created the issue and assigned it to you. Thanks!

@edwinthinks edwinthinks merged commit a467dcb into main Oct 29, 2022
@seanmarcia seanmarcia deleted the partner-social-media-changes branch June 14, 2023 13:48
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.

Partner social media changes
3 participants