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

Only allow submission of Leaders portion of form if a company is marked as "Leaders Eligible" via label #17

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

Conversation

Shinsina
Copy link
Member

@Shinsina Shinsina commented Nov 17, 2022

Portal Homepage NOT LEADER:
Screenshot from 2022-11-17 12-06-29

Portal Homepage LEADER:
Screenshot from 2022-11-17 12-06-41

Following submission of Leaders portion if not eligible:
Screenshot from 2022-11-18 14-51-41
Screenshot from 2022-11-18 14-54-17

This would only occur if somehow a user navigated to the Leaders portion of their respective Company Update form (which would have to be done via the URL as there isn't a button presented to them to be directed to it) and attempted to submit that portion.

@Shinsina
Copy link
Member Author

Copy link
Member

@solocommand solocommand left a comment

Choose a reason for hiding this comment

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

@Shinsina There are a couple of structural issues to address, but this must be an opt-in configurable option, not a change made across the board. Existing customers have this already deployed with expected functionality, and at least one (AB Media) uses it to add companies to the leaders program, not just update the selections.

The out-of-the-box solution that is supposed to be used for selecting which companies are allowed to see the leadership submission segment is the leadershipCompanyLabel config.

services/app/app/controllers/portal/index.js Show resolved Hide resolved
services/app/app/gql/queries/portal.graphql Outdated Show resolved Hide resolved
services/app/app/templates/portal/leadership.hbs Outdated Show resolved Hide resolved
@Shinsina
Copy link
Member Author

@Shinsina There are a couple of structural issues to address, but this must be an opt-in configurable option, not a change made across the board. Existing customers have this already deployed with expected functionality, and at least one (AB Media) uses it to add companies to the leaders program, not just update the selections.

The out-of-the-box solution that is supposed to be used for selecting which companies are allowed to see the leadership submission segment is the leadershipCompanyLabel config.

@solocommand Want to start out in saying I ultimately don't care how we handle this but I want the reasoning/rationale and most importantly the expected behavior provided a specific company criteria to be consistent, my understanding via previous discussions was to "make the form logic and the site logic the same for determining who is or is not a leader" and this would be doing that.

The leaderCompanyLabel concept is only found within this form and doesn't provide any sort of qualifications elsewhere as to "who is or is not a leader" which adds potential points of confusion as to "who is what and how". If ultimately we would rather use labels for determining that logic, my personal opinion would be to do that everywhere but as it stands currently that isn't how it works and I understand there is more to it than just flipping logic one way or the other.

Additionally, I wasn't able to find anyone using leadershipCompanyLabel, it's possible I missed it in glancing over the Rancher configurations for this, if you could let me know by whatever means as to who is currently using the form in that fashion, would be much appreciated.

Ultimately another additional configuration option to opt into this behavior (showing leaders only to companies that meet isLeaders criteria) is fine but I would rather we use existing behavior (such as the labels) than creating new behavior (which is what this is currently doing at least from the form side).

@Shinsina Shinsina requested a review from solocommand November 28, 2022 14:52
@Shinsina
Copy link
Member Author

@solocommand In the current state I've opted to just use the existing label configuration option, this separates the logic of "Leaders Eligible" from "Is a Leader" entirely so as not to cause potential points of conflict or confusion as to who gets what and how between this and the site.

@Shinsina Shinsina changed the title Only display Leadership pages/options if a company is marked as a Leader Only allow submission of Leaders portion of form if a company is marked as "Leaders Eligible" via label Nov 28, 2022
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