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

dev/core#1511 Expose option to show event location on configuration tab #16230

Merged
merged 2 commits into from
Jan 16, 2020

Conversation

aydun
Copy link
Contributor

@aydun aydun commented Jan 8, 2020

Overview

Add a checkbox on the Event Location configuration tab to control whether the location is displayed to users.

The functionality already exists and the field can be configured by API. This enables configuration via the GUI.

Before

The option can only be configured by API.

After

Option can also be configured via GUI.

image

Technical Details

The logic for the use of the option already exists. This change just exposes it via GUI.

Comments

See https://lab.civicrm.org/dev/core/issues/1511

@civibot civibot bot added the master label Jan 8, 2020
@civibot
Copy link

civibot bot commented Jan 8, 2020

(Standard links)

@yashodha
Copy link
Contributor

yashodha commented Jan 9, 2020

@Adyun looks good, tested this and it works fine.

@mattwire
Copy link
Contributor

mattwire commented Jan 9, 2020

@aydun The only issue I have here is reading "Show Location?" I have no idea where it is actually shown. If you can clarify via text/description/help text then I'm happy to merge this.

@yashodha
Copy link
Contributor

@mattwire @aydun Would it make sense it to provide the event info link (where the location will be showed or not) so that it is more clear. Something like

Show location here

@aydun
Copy link
Contributor Author

aydun commented Jan 10, 2020

It affects the location in a few places - I'll add a help note.

@aydun
Copy link
Contributor Author

aydun commented Jan 13, 2020

Help text added - please re-review.

<li>the Event Information page</li>
<li>event registration confirmation emails</li>
<li>the HTML, RSS and ICALENDAR feeds of event information</li>
</ul>
Copy link
Contributor

@yashodha yashodha Jan 15, 2020

Choose a reason for hiding this comment

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

@aydun
Can we remove the in first and last and capitalise all or none

  • Event Information page
  • Event registration confirmation emails
  • HTML, RSS and ICALENDAR feeds of event information

@aydun aydun force-pushed the 1511_event_show_location branch from 0c0777f to 22cf2ac Compare January 15, 2020 15:09
@eileenmcnaughton
Copy link
Contributor

eileenmcnaughton commented Jan 15, 2020

(CiviCRM Review Template DEL-1.2)

  • General standards
    • Explain (r-explain)

      • PASS : The goal/problem/solution have been adequately explained with a link (JIRA, Github, Gitlab, StackExchange).
    • User impact (r-user)

      • PASS: The change would be intuitive or unnoticeable for a majority of users who work with this feature.
    • Documentation (r-doc)

    • Run it (r-run)

      • PASS: We tested this & setting this variable altered how it shows up on the info screen. We weren't able to figure out how to test that it shows on the email - on the test site
    • Technical impact (r-tech)

      • PASS: The change preserves compatibility with existing callers/code/downstream.
    • Code quality (r-code)

      • PASS:
    • Maintainability (r-maint)

      • PASS: The change sufficiently improves test coverage, or the change is trivial enough that it does not require tests because it is only the tpl layer
    • Test results (r-test)

      • PASS: The test results have failures, but these have been individually inspected and found to be irrelevant.

@eileenmcnaughton
Copy link
Contributor

test this pleasee

@seamuslee001
Copy link
Contributor

Test failures unrelated

@seamuslee001 seamuslee001 merged commit 077980f into civicrm:master Jan 16, 2020
@homotechsual
Copy link
Contributor

Will this drop in 5.23 or 5.24 -> I suspect 5.24 but am not always entirely sure...

@eileenmcnaughton
Copy link
Contributor

5.23 - 5.22 is the current rc so itt will hit the next one

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants