CheatSheets: Add check for IA page existence

[GuiltyDolphin]

What does your Pull Request do (check all that apply)?

Choose the most relevant items and use the following title template to name
your Pull Request.

• New Instant Answer
  • Cheat Sheets: New {Cheat Sheet Name} Cheat Sheet
  • Other: New {IA Name} Instant Answer
• Improvement
  • Bug fix: {IA Name}: Fix {Issue number or one-line description}
  • Enhancement: {IA Name}: {Description of Improvements}
• Non–Instant Answer
  • Other (Role, Template, Test, Documentation, etc.): {GoodieRole/Templates/Tests/Docs}: {Short Description}

Description of changes

Provide an overview of the changes this pull request introduces.

Which issues (if any) does this fix?

Fixes #3016 - we now check the IA page exists.

People to notify (@mention interested parties)

@zachthompson @jdorweiler

---

Instant Answer Page: https://duck.co/ia/view/cheat_sheets

Maintainer: @zachthompson

[daxtheduck]

Cheat Sheets (Controller)

Description: The base controller for all Cheat Sheet Goodies. This is the actual Instant Answer that gets triggered, and decides which Cheat Sheet to display based on the query. This Goodie also handles the parsing of each Cheat Sheet's JSON and the display of the Cheat Sheet.

Example Query: [vim cheatsheet](https://beta.duckduckgo.com/?q=vim cheatsheet)

Tab Name: Cheat Sheet

Source:

These are the important fields from the IA page. Please check these for errors or missing information and update the IA page

---

This is an automated message which will be updated as changes are made to the IA page

[GuiltyDolphin]

@zachthompson @jdorweiler As you can see, some cheat sheets do not have pages (it works)!

[jdorweiler]

👍 We will have to fix those before merging though.

[GuiltyDolphin]

@jdorweiler Yeah - there might be some IA pages already with the wrong name; in which case we might just want to rename those to line up with the cheat sheets.

[GuiltyDolphin]

@zachthompson @jdorweiler This is passing now - yea happy with it?

My only concern is that this probably won't pick up any IAs in planning, seeing as they won't be in the metadata.

[zachthompson]

Thank you.

[mintsoft]

@zachthompson @GuiltyDolphin this appears to have made it more awkward to actually work on new cheatsheets. One of my colleague was attempting to do his first cheat-sheet last night and has reported great frustration.

If a user is creating a cheat-sheet without a PR (i.e. when they're developing it initially); then when running duckpan test the tests fail, his cheat sheet is still in planning phase from the IA (https://duck.co/ia/view/notepad_plus_plus_cheat_sheet) perspective.

I'm not sure if it's this PR that's made this difficult however it seems suspiciously linked?

[GuiltyDolphin]

@mintsoft Yup - it is this PR (hence my last comment). I've got an issue open to include planning IAs in the metadata (duckduckgo/community-platform#1354); once that's sorted this should be fixed.

@zachthompson We could hack in a temp fix by adding a --no-meta flag to the tests, then implicitly call this in DuckPAN but not in the Travis build; though I think it might be wasted work considering the alternatives are probably more acceptable (updating metadata).

[mintsoft]

@GuiltyDolphin I don't think that link is the right one?

[GuiltyDolphin]

@mintsoft Whoops yeah - wrong repo; I've updated it.

[zachthompson]

@mintsoft It's really not designed for incremental testing but doing it when you think you might be ready to submit. No metadata = not ready to test. Your friend must be in development if there's anything worth testing.

[mintsoft]

@zachthompson and how do you get it into development without a PR? The flow doesn't seem quite right to me

[GuiltyDolphin]

@mintsoft Yeah - it's a bit weird. Ideally we should be able to tell the platform that an IA is in development through DuckPAN:

• Create the IA Page (planning)
• Start development locally (development)
• PR (development & testing)
• Merge (complete)
• Release (live)

[mintsoft]

@zachthompson @GuiltyDolphin We had another report of this in slack; someone was confused why their tests were failing in dev

https://duckduckhack.slack.com/archives/ask-anything/p1465167951000048

[zachthompson]

@mintsoft Let me make sure I understand right:

1. A dev creates and IA page which defaults to "planning"
2. They do some work and test
3. It fails because it's not in the feed
4. Yet, they can't update the status to "development" without asking an admin
5. Fist clenching

[mintsoft]

Exactly @zachthompson

[GuiltyDolphin]

@zachthompson Any update on duckduckgo/community-platform#1354? Once that's sorted I imagine the issue will resolve itself.

[zachthompson]

Nothing yet. I did mention this type of case in our task for it. It's just a matter of prioritization, unless one of you wants to take a look at the necessary community platform change.

[zachthompson]

We discussed this a bit this morning. It's not as simple as adding IAs in planning to the feed since we have a fair amount of spam. Allowing users to update the status to development suffers the same problem.

However, @MariagraziaAlastra tells me that the ID will be moved to development when you create a PR. If you're at the point of needing to test this probably should have been done already. In the near term that's the solution.

Let us know if that isn't working and we can check it out further.

[mintsoft]

Hmm I dunno if I agree with that; personally my workflows are create failing test, write code, run test; if I have to do a PR before I can actually test things, that seems backwards to me? Granted I guess this only applies to cheatsheets, but if it was pushed out for all new IA's that seems really suboptimal.

[zachthompson]

I don't know if I follow what you're saying about "create failing test". That sounds to me like existing code to which this usually doesn't apply. We have tried to communicate that we would like PRs as early as possible to:

1. Prevent duplicate work
2. Review the progress of the implementation
3. For larger changes, evaluate whether the direction is one we want to take.

I really see no advantage of being able to run tests on these behind closed doors. The metadata is critical to these working.

[GuiltyDolphin]

@zachthompson I agree with @mintsoft - I won't (usually) create a PR until I have something that is at least working; other discussion usually happens in issues.

I think it is rather restrictive on local developers to require that a PR is created before a working IA can be made.

Why not allow users to enter IAs into the development stage manually (update the status)? With a maximum number of 'in-development' IAs without a PR per user?

Duplication in new IAs is rare; it usually only happens when an issue has been created and people taking them on without saying so.

[mintsoft]

You make a fair point; I'm not saying that all work should be done in the dark as it were. I may be overarching for this context. In this context yes; I understand that test driving doesn't directly apply, I wouldn't necessarily expect people to run tests prior to a PR for a cheatsheet. However, that said, this has come up twice without prompting and caused confusion for some noobs? If they're really new then it seems somewhat unfriendly for them to be placed in a position of attempting to figure out what they've done "wrong" when there's nothing technically wrong.

Maybe the problem is that people are under the impression that they should only create a PR when "it's done"; rather than "as they're doing it"? I'm not too sure how to instil open working in this situation

@GuiltyDolphin thoughts?

[GuiltyDolphin]

@mintsoft As I said previously - I'm something of a 'local' dev.

Hmm, not sure about your point about not having run tests though, don't we stress this in the docs?

I think if we're going to make PRs mandatory, we should make it really easy to create a PR with everything filled out for you, ideally directly from the platform; otherwise we're just adding overhead to the development process...

[mintsoft]

Hmm, not sure about your point about not having run tests though, don't we stress this in the docs?

We do normally; although I've not checked the cheatsheetguide. I'd imagine that it does tell them to run tests

[zachthompson]

@mintsoft I think you're right about the PR impression. We probably just need more consistent message around it. This is more important for full-blown IAs, not so much for cheat sheets.

@GuiltyDolphin You shouldn't need to run the t/CheatSheets test throughout developing a cheat sheet. Maybe it's just me but I've written a fair number of them and have never felt the need to repeatedly run the parent tests. In fact, I usually let travis have the first crack at running those which is perfectly fine in this case.

Opening a PR like you would normally, filling in as much as you know about the cheat sheet, and marking it WIP is really no more effort. And this is still only if you really feel compelled to run the parent tests on your json file before opening a PR. I don't think that qualifies as mandatory.

[zachthompson]

A refinement to the test might be to do the metadata tests only if it's in the feed. Otherwise just warn that it isn't and that an IA page/PR will eventually need to be created.

[GuiltyDolphin]

@zachthompson Hmm, I'm mostly referring to what the situation might be like with full Goodies; I don't really make many cheat sheets.

Even so, I simply bat out rt in Vim and the tests run so it's not really a mental effort; mostly automatic when making changes.

I agree - we should have some distinction between local and Travis builds.

What do you think of having an env variable for this? Locally, we could produce a warning with no IA page, but on Travis set the variable and have tests fail if there is no page?

[mintsoft]

What do you think of having an env variable for this? Locally, we could produce a warning with no IA page, but on Travis set the variable and have tests fail if there is no page?

I came to the same conclusion; Travis should test this as part of the merge procedure, however I don't think it should prevent the use locally

[zachthompson]

@GuiltyDolphin That's another idea. Where outside of cheat sheets do we run into this?

[GuiltyDolphin]

@zachthompson Nowhere - yet; but I imagine we'll want to integrate metadata with development more closely in the future? Seeing as metadata defines useful information such as example queries and (potentially) files.