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

Gate links to teacher application on gatekeeper flag #42615

Merged
merged 5 commits into from Sep 23, 2021

Conversation

tim-dot-org
Copy link
Contributor

@tim-dot-org tim-dot-org commented Sep 22, 2021

This PR adds some conditional logic to the regional partner search page, which is where all the links to the application are. When we flip the pd_teacher_application Gatekeeper flag, the teacher application is a 404 to non-workshop admins, so we shouldn't link to it anywhere.

Links

  • slack convo from 2 yrs ago about archiving applications: thread
  • jira ticket: PLAT-1220

Testing story

Tested locally with hardcoded flags.

Flag off:
image

Flag turned on:
image

@tim-dot-org tim-dot-org requested a review from a team September 22, 2021 00:04
@davidsbailey
Copy link
Member

davidsbailey commented Sep 22, 2021

@tim-dot-org I'm seeing a bit of a disconnect between the conclusion in the slack thread and the description here. the thread seemed to suggest making the teacher application url keep working:

Marina 2 years ago
delayed follow up here...but yes, it was an intentional decision to only close applications on the website and not in the application itself to reduce the work on our end since most of the entry points go through our site.

question 1: am I misunderstanding the thread? and/or is there more recent guidance your following which says to make the teacher application itself 404?

@davidsbailey
Copy link
Member

another top-level comment here is that it seems like we could really use a doc, similar to Shipping curriculum for 2021/2022, to document what needs to happen each year.

Question 2: Does anything like this exist (@tess323 maybe)?

usual caveat: I'm new to this part of the codebase, so if this seems over the top please feel free to say so... I just get a bit worried when an old slack thread is the only form of documentation for an annual change like this.

@davidsbailey
Copy link
Member

Question 3: are we in a situation where certain code gets added / removed on an annual basis? again I'm not super familiar with the details here, but I'm not sure I understand why there is so much code change for something that seems more like an annual config change.

@tim-dot-org
Copy link
Contributor Author

  1. Yeah, actually that slack convo is a little misleading, I had a conversation with Tess and we decided that it would make sense to completely take down the application for non-admins due to the number of breaking changes we'd be introducing to the application this year as opposed to previous years.
  2. I'll start a doc for this! The thing about the docs is that it doesn't feel like there's a good place to link to them from, so you end up needing to search for them like slack conversations anyways.. Maybe a readme file in the codebase would be a better solution (both interlinked probably is best)? I think this is the first time Tess has been doing this change also, since I think we did not close applications (or at least didn't make any major changes) last year.
  3. It seems to me like these links were just missed when considering what needed to be removed when closing applications. I think our process for closing applications has evolved over the years, where initially, it was entirely based on the RP's app close date, and now that we're making larger changes we want to be able to close them across the board. In this code change, I'm just making it a little more future proofed so that we don't need to make annual changes and can just flip the flag to toggle all of this functionality at once.

Thanks for your thoughts!

@davidsbailey
Copy link
Member

Thank you Tim, this is super helpful! Thanks for entertaining my semi-uneducated questions. I feel good about 1 and 3. as for the doc, I think it is a major problem we have across the eng team that there's no standard way to store documentation in a way that is easily discoverable. a markdown file in the codebase is great if only engineers need to interact with it, and I love how it can be updated as part of a PR. however, if partners need to be able to edit and comment, then it could be better to have a google doc, though, and we can figure out as we go where best to link to it from.

I'm not sure if this applies to things on the PLC side, but you can see from Shipping curriculum for 2021/2022 that we have a fair bit of discussion in there with non-eng stakeholders. If the PLC world is similar, then I'd suggest going with a google doc -- but you probably have best perspective here and I'd be kind of interested to try out the markdown file in github option if that seems better to you.

cc @tess323 again for input on the format of any new documentation here.

@tim-dot-org
Copy link
Contributor Author

I took out the actual archiving commit (bumping the current application year constant) because it'll be difficult to do that without the application refactor PR. So order of operations now:

  • Merge this PR (removes links to application)
  • Close applications (toggle gatekeeper pd_teacher_applications value)
  • Merge application refactor (making it easier to test and reference the latest application class)
  • Archive 21-22 applications by bumping the constant

@tim-dot-org tim-dot-org changed the title Archive applications 2021 Gate links to teacher application on gatekeeper flag Sep 22, 2021
nominated={this.state.nominated}
priorityDeadlineDate={appsPriorityDeadlineDate}
/>
)}
Copy link
Member

Choose a reason for hiding this comment

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

it looks like while the ajax request is pending, or if it fails, we'll show the StartApplicationButton, even if applications are closed. Is that correct? If so, I would suggest a solution which never indicates that applications are open when they are actually closed. one idea would be to invert the api logic to be applications_open instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch! changed

@tim-dot-org tim-dot-org merged commit ca373aa into staging Sep 23, 2021
@tim-dot-org tim-dot-org deleted the archive-applications-2021 branch September 23, 2021 20:53
@tess323
Copy link

tess323 commented Sep 23, 2021

@davidsbailey @tim-dot-org - weighing in on the documentation side. This is my first rodeo on archiving applications so I don't think that there is heavy process around this yet. I agree this is probably a good one for a google doc until we get everything locked in. Thank you for starting that @tim-dot-org!

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.

None yet

3 participants