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

Smoking outside tuning #3865

Merged
merged 17 commits into from
Mar 23, 2022
Merged

Conversation

mnalis
Copy link
Member

@mnalis mnalis commented Mar 10, 2022

This PR tries to address superfluous OUTSIDE answer when it should be hidden, as reported in #3856

  • do not show quest at all if there are neither indoor nor outdoor seating (nor smoking area)
  • hide OUTSIDE answer if it is known that there is no smoking possible outside (no outside seating, and no standing smoking area like in nightclub)

@peternewman
Copy link
Collaborator

* hide `OUTSIDE` answer if it is known that there is no outside seating

In the UK at least it's quite common, particular outside nightclubs, to have a small fenced area where people can smoke without having to leave the area the club controls (and pay for a new ticket etc). This fenced area is often on the pavement and may not have seating, which might break your current logic here.

See some average to rubbish photos of the like here:
https://www.vice.com/en/article/z3xwa4/this-is-what-englands-nightclub-smoking-areas-looked-like-last-night

@mnalis
Copy link
Member Author

mnalis commented Mar 11, 2022

In the UK at least it's quite common, particular outside nightclubs, to have a small fenced area where people can smoke without having to leave the area the club controls

thanks @peternewman that is quite informative! I'm now undecided, should I just add the exception for nightclub (and likely stripclub as it might behave similarly?), or do pubs/cafes/restaurants also might have such arrangements and I should thus show OUTSIDE answer regardless of outdoor_seating=no ?

@peternewman
Copy link
Collaborator

thanks @peternewman that is quite informative! I'm now undecided, should I just add the exception for nightclub (and likely stripclub as it might behave similarly?), or do pubs/cafes/restaurants also might have such arrangements and I should thus show OUTSIDE answer regardless of outdoor_seating=no ?

Maybe pubs too as it might apply to some of the more clubby/youth ones, its more about having to queue for entry, possibly pay and/or have ID checks.

I don't think cafes or restaurants will have the queue/pay/ID issues, so probably won't bother. They're also more likely (possibly along with some pubs) to have outdoor seating at which point that bit kicks in too.

@mnalis
Copy link
Member Author

mnalis commented Mar 17, 2022

When you work it out, it would be good to add some tests for that (if they don't exist already), to ensure it continues to behave in the same way in case this or something else is making use of that behaviour.

I've created app/src/test/java/de/westnordost/streetcomplete/quests/smoking/AddSmokingTest.kt and tried to put some checks there. What else is needed so that those tests are checked? Are the checks run automatically when package is being built when click on .github/workflows/build-debug-apk.yml GitHub actions or do something special needs to be done?

mnalis and others added 3 commits March 18, 2022 21:53
…/AddSmokingTest.kt


group test together

Co-authored-by: Flo Edelmann <florian-edelmann@online.de>
…/AddSmokingTest.kt


group test together

Co-authored-by: Flo Edelmann <florian-edelmann@online.de>
…/AddSmokingTest.kt


fix nightclub test

Co-authored-by: Flo Edelmann <florian-edelmann@online.de>
@mnalis
Copy link
Member Author

mnalis commented Mar 18, 2022

Thanks for suggestions, @FloEdelmann! Do you know how/when those tests are actually run?
I'd like to run them to verify it it works as intended, before moving this out of draft.

@FloEdelmann
Copy link
Member

The tests are not run at all on GitHub Actions, they have to be run locally: Either via Android Studio, or in the commandline:

./gradlew :app:cleanTestDebugUnitTest :app:testDebugUnitTest --tests "de.westnordost.streetcomplete.*"

mnalis added a commit to mnalis/StreetComplete that referenced this pull request Mar 18, 2022
@mnalis
Copy link
Member Author

mnalis commented Mar 18, 2022

Thanks @FloEdelmann , I've added that command (in my fork GitHub workflow) to run tests, and it passes the checks now.

One more question: do you know if it is possible to check whether some option is shown in the list of answers or not?

Smoking quest sometimes offers OUTSIDE answer (and sometimes not) depending on various conditions (i.e. https://github.com/mnalis/StreetComplete/blob/smoking-outside-fix/app/src/main/java/de/westnordost/streetcomplete/quests/smoking/SmokingAllowedAnswerForm.kt#L25), so I'd like to check that too if it is (easily) possible.

@FloEdelmann
Copy link
Member

do you know if it is possible to check whether some option is shown in the list of answers or not?

Unfortunately, I don't know if that's possible here. Maybe only in the instrumented tests that have access to the Android runtime.

@mnalis
Copy link
Member Author

mnalis commented Mar 19, 2022

Thanks @FloEdelmann, I though that it might be more complicated... Then this is ready for review.

@mnalis mnalis marked this pull request as ready for review March 19, 2022 18:24
Copy link
Member

@westnordost westnordost left a comment

Choose a reason for hiding this comment

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

Looks good!

@westnordost westnordost merged commit f4996f1 into streetcomplete:master Mar 23, 2022
@mnalis mnalis deleted the smoking-outside-fix branch March 23, 2022 19:34
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

4 participants