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

Adds phone number and extension to typeahead #585

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

seagoj
Copy link

@seagoj seagoj commented Jun 18, 2013

Adds an option to enable pre-population of the phone number and extension when creating a ticket.

@seagoj
Copy link
Author

seagoj commented Jul 25, 2013

I imagine you guys have more pressing issues, but it seems silly to not store a phone number if it was typed in. If you're not going to accept the pull request. Could you give me a quick justification?

Thanks

@ntozier
Copy link

ntozier commented Jul 25, 2013

I can think of three good reasons:

  1. staff who work at multiple sites and thus have multiple primary numbers numbers.
  2. staff who some times work from home.
  3. staff who need to specify an alternative (cell, hotel, etc) number because their primary phone is dead or they are traveling.

If users do not have to change it they wont.

@Richiricheh
Copy link

I think that given the number of mods we have seen throughout the years in the mod forums there are quite a few people who'd love to have this as an option and would have it enabled by default. Personally I think this needs to be an option which can be enabled/ disabled from the admin panel.

@ntozier I think you've brought up a interesting points and they should be considered as I have heard whispers that OStickets roadmap is potentially moving towards a profile-based UID + PWD approach (I am very much out of the loop so that might have since changed).

@seagoj
Copy link
Author

seagoj commented Jul 25, 2013

@ntozier I agree that having multiple phone numbers show up can be frustrating, but I think too much information is better than none at all. At least you have a chance of contacting the submitter if the phone number is retained.

I know everyone would use the system differently, but having the email address is nice for keeping a record of what happens, but actually being able to call is usually what gets the job done most expediently.

@seagoj
Copy link
Author

seagoj commented Jul 25, 2013

@Richiricheh I agree that it should be an option in the admin panel. I just did this quick and dirty for my own purposes. :)

In fact, that's partially why I commented on it again. If it was something people were interested in, I would happily flesh it out and make it an option (either on by default or no).

@Richiricheh
Copy link

I suppose my comment was made due to the valid concerns brought forward by @ntozier. If it was a toggled option in the admin settings then I truly cannot think of a reason why it shouldn't be added as part of the project because frankly for those which want the manual phone entry then they will leave the toggle off and for those (like you and I as well as a bunch of people from the MOD forums) we would leave it toggled ON.

@seagoj
Copy link
Author

seagoj commented Jul 26, 2013

OK, this commit makes it an option in the admin settings.

It's still a bit ugly, though. I couldn't figure out the best way to access the $cfg in ajax.users.php, so it makes a separate db query. If anyone would like to point me in the right direction on how to accomplish that, I would appreciate it.

Thanks

@seagoj
Copy link
Author

seagoj commented Aug 1, 2013

Any help or is the additional call to the database the way to go?

@greezybacon
Copy link
Contributor

@seagoj, you should be able to:

global $cfg;
$cfg->getStorePhone();

@seagoj
Copy link
Author

seagoj commented Aug 6, 2013

Thanks for the help! I've edited ajax.users.php to just pull the value straight from CFG.

@seagoj
Copy link
Author

seagoj commented Aug 6, 2013

Please let me know if there's anything else that needs to be done.

@@ -183,7 +183,8 @@ INSERT INTO `%TABLE_PREFIX%config` (`namespace`, `key`, `value`) VALUES
('core', 'admin_email', ''),
('core', 'helpdesk_title', 'osTicket Support Ticket System'),
('core', 'helpdesk_url', ''),
('core', 'schema_signature', '');
('core', 'schema_signature', ''),
('core', 'store_phone', '1');
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bad idea. It implies a database migration. You'll also need to change INCLUDE_DIR/upgrader/streams/core.sig to be the MD5 hash of the updated install-mysql file. And you'll need to write a migration script for the INCLUDE_DIR/upgrader/streams/core/ folder to migrate everyone's database to add this new setting.

We refactored the config class again. Inside the osTicketConfig class in class.config.php, there is a $defaults array that provides defaults for configuration not yet stored in the database. I would like to see

'store_phone' => 1

added to the defaults array. This method doesn't require a database migration and offers the same initial default

@greezybacon
Copy link
Contributor

@seagoj, have you used git rebase before?

@seagoj
Copy link
Author

seagoj commented Aug 9, 2013

@greezybacon I have before. What would you like me to do?

@greezybacon
Copy link
Contributor

What we've got here is just really messy. 21 commits for a simple feature of 40 lines of code. Any way you could condense it into just a couple commits? And drop the search_phone default from the abstract Config class.

@seagoj
Copy link
Author

seagoj commented Aug 11, 2013

Ok, it's been rebased down to 1 commit and I fixed the errant search_phone in the Config class. Thanks for the help with this, by the way.

@greezybacon
Copy link
Contributor

I implemented this in osTicket/osTicket#2. I think this will be the best way to get it implemented.

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.

4 participants