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

Behaviour re social media on submit for approval #3211

Closed
1 of 2 tasks
cielf opened this issue Oct 26, 2022 · 24 comments · Fixed by #3651
Closed
1 of 2 tasks

Behaviour re social media on submit for approval #3211

cielf opened this issue Oct 26, 2022 · 24 comments · Fixed by #3651
Labels
core team Groomed but likely needs expert knowledge

Comments

@cielf
Copy link
Collaborator

cielf commented Oct 26, 2022

Summary

When the partner clicks "submit for approval", they should get an error if they have not either indicated "no social media presence" or given info for at least one social media (i.e. website, instagram, facebook, twitter)

Things to consider

This issue follows on to the PR Partner social media changes
#3148

Criteria for Completion

[Assuming the changes in PR #3148 are in...]

  • When a partner tries to submit their profile for approval, and they have no social media filled in, and have not indicated that they have "no social media presence", they should get an error.
  • tests to support this functionality
@Learningstuff98
Copy link
Collaborator

@cielf I have an update. Its ready except for one test that I'm having an issue with. Its the system test that I've commented out for now. Everything is behaving properly in localhost, but the opposite is happening when the test runs. In other words when the test runs with invalid media information, instead of the page getting an error like it should, the submission is going through.

In localhost:

Screenshot (596)

From the system test:

failures_r_spec_example_groups_approval_process_for_partners_request_approval_with_invalid_details_should_render_an_error_message_836

Here is a link to what I have so far:
link

Any advice would be appreciated. Also I can delete this test if its too much of a hassle.

@cielf
Copy link
Collaborator Author

cielf commented Nov 3, 2022

Hey @Learningstuff98 -- I'm actually on vacation (staycation, but still) this week -- I'm going to send myself a reminder for this for when I'm back.

@cielf
Copy link
Collaborator Author

cielf commented Nov 7, 2022

Hey @Learningstuff98 -- i just manually tested this on my local, and it fails in the same way as the test fails. The steps:
1/ signed in as organization admin
2/ Added a new partner - (my own email address)
3/ When the email flashed up, I copied the link and opened it in a different browser
4/ I then went to "My organization", and clicked "Submit for Approval"

The submission went through.

Totally as a side note: you can mark a test to be skipped -- you don't have to comment it all out.

@cielf
Copy link
Collaborator Author

cielf commented Nov 7, 2022

Hmm. Hey @Learningstuff98 Just looking at the validations in partners/partner.rb. I don't see a dependence on the status -- which I would have expected.

@cielf
Copy link
Collaborator Author

cielf commented Nov 7, 2022

@Learningstuff98 So... more than one thing here, I think. if I add a check for partner status into the acceptance validation in partners/partner.rb, you still won't get the results you want -- But, you will have partner.profile.errors containing something after line 17 in request_approval_service.

@cielf
Copy link
Collaborator Author

cielf commented Nov 7, 2022

@Learningstuff98 I don't know why it's not catching it -- maybe some kind of optimization so that if there is no change in the conditions since the last save, it doesn't fire? I think we might need to call in someone with deeper rails understanding than me to fully understand that. (looks at @dorner or @edwinthinks)

@Learningstuff98
Copy link
Collaborator

@cielf Yeah honestly I'm stumped. I agree with your idea of getting some extra eyes on this.

@cielf
Copy link
Collaborator Author

cielf commented Nov 8, 2022

@Learningstuff98 (nods) If we split things up in the ApprovalRequestService so that it checks if it is submitted already, then update the partnerstatus, then check validity, we can make it work, I believe. I just don't grok why it's reading as valid in the first place.

@dorner
Copy link
Collaborator

dorner commented Nov 8, 2022

I'd have to check out the code to take a look. Nothing jumps out at me by looking at it.

@Learningstuff98
Copy link
Collaborator

I have an update. I got it to the point where everything works in the browser on my end and the system test is behaving properly, however I've got other tests that are now failing. I hope to have everything ready for a PR soon once I've addressed the current failing tests.

@Learningstuff98
Copy link
Collaborator

@cielf I've resolved the issue with the other failing tests, so its just about ready for a PR except for one last thing. Could you pull down the recent changes that I've sent to this branch and test it through the flow that you mentioned earlier? I would test it myself but I'm not getting the email with the link that you mentioned. I expect it to work as it should now.

@github-actions
Copy link
Contributor

This issue has been inactive for 258 hours (10.75 days) and will be automatically unassigned after 102 more hours (4.25 days).

@github-actions
Copy link
Contributor

This issue has been inactive for 378 hours (15.75 days) and is past the limit of 360 hours (15.00 days) so is being unassigned.

@scooter-dangle
Copy link
Collaborator

Discussion: We think a fix was merged. Need to confirm.

@cielf
Copy link
Collaborator Author

cielf commented Dec 19, 2022

Hmm -- I think the fix we were thinking of did 90% of what was needed, and this is for the other 10%, but will investigate further.

@cielf
Copy link
Collaborator Author

cielf commented Dec 19, 2022

The fix is in (though I just found a problem with it) for when you are editing the profile, but it is not for when you submit for approval. That's what this issue is actually about.

@cielf
Copy link
Collaborator Author

cielf commented Dec 19, 2022

@Learningstuff98 I'm so sorry -- this fell out of my attention. I've taken it through the flow [new partner, invite, submit for approval], and it is behaving the way that I expected, and the test suite passes. It will require some work to merge it with the current main -- because there's been more work on the partner merge in the meantime, but I think you should submit your PR. Please be aware that we are currently on a merge freeze until Jan 1.

@Learningstuff98 Learningstuff98 self-assigned this Dec 20, 2022
@github-actions
Copy link
Contributor

This issue has been inactive for 259 hours (10.79 days) and will be automatically unassigned after 101 more hours (4.21 days).

@github-actions
Copy link
Contributor

github-actions bot commented Jan 5, 2023

This issue has been inactive for 379 hours (15.79 days) and is past the limit of 360 hours (15.00 days) so is being unassigned.

@cielf
Copy link
Collaborator Author

cielf commented Jan 20, 2023

I was asked to check the current state of this. The branch passes my manual tests and passes the current suit of automated tests on my local. I think all it needs is to be rebased on the current main -- but there have been a lot of changes in the meantime -- including the whole partner merge thing, so that will probably require a core team member.

@cielf
Copy link
Collaborator Author

cielf commented Jan 20, 2023

I'm somewhat willing to take that on, but I want to wait until after the counties stuff gets merged in -- this touches a lot of the same files.

@cielf cielf added the core team Groomed but likely needs expert knowledge label Jan 30, 2023
@cielf
Copy link
Collaborator Author

cielf commented Jun 9, 2023

I totally missed mentioning that this should only be checked if the organization uses the media portion of the profile. Mea Culpa.

@dorner dorner self-assigned this Jun 11, 2023
@github-actions
Copy link
Contributor

This issue has been inactive for 241 hours (10.04 days) and will be automatically unassigned after 119 more hours (4.96 days).

@github-actions
Copy link
Contributor

This issue has been inactive for 361 hours (15.04 days) and is past the limit of 360 hours (15.00 days) so is being unassigned.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core team Groomed but likely needs expert knowledge
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants