Add user interface option to Notify by Pokemon Level #2399

tomballgithub · 2017-12-16T17:16:16Z

Description

This adds a user interface option to notify by pokemon level. This is virtually the same coding as the notify by pokemon perfection.

If the user enters a pokemon level between 1 and 40, the map will notify them if a pokemon meeting that level is found. If both a level and perfection notification are configured, a pokemon will only be notified if it meets both criteria.

Motivation and Context

This was request in the PR channel and I have had multiple users request this feature. It is especially interesting now with weather-boosting causing pokemon with level 35 to be found in the wild

How Has This Been Tested?

I tested the code on my Rocketmap server user interface under Ubuntu 16.04.
I tweaked it until it worked as desired.

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.

sLoPPydrive · 2017-12-19T17:23:48Z

static/js/map.js

        hasHighIV = notifiedMinPerfection > 0 && perfection >= notifiedMinPerfection
+        hasHighLevel = notifiedMinLevel > 0 && level >= notifiedMinLevel
+
+        hasHighAttributes = (hasHighIV && notifiedMinLevel === 0) || (hasHighLevel && notifiedMinPerfection === 0) || hasHighLevel && hasHighIV


Change this to

hasHighAttributes = (hasHighIV && !(notifiedMinLevel > 0)) || (hasHighLevel && !(notifiedMinPerfection > 0)) || hasHighLevel && hasHighIV

...to make it always work.

tomballgithub · 2017-12-19T20:44:01Z

FYI sloppydrive's users found the needed code improvement earlier but otherwise this is working well.

sirlindqvist

No problems, also works good in combination with minimum IV level.

darkelement1987 · 2018-01-15T06:39:48Z

Is it also possible for you to build in 'Exclude/hide pokemon by iv/level'? I would love that.

tomballgithub · 2018-01-15T18:02:20Z

@darkelement1987 Yes there is another PR that allows you to hide pokemon that are not being notified. So you could set the minimum level and IV to what you want and then 'hide unnotified pokemon' and it will only show the high level ones (excluding the others) #2377

darkelement1987 · 2018-01-15T18:55:13Z

Awesome! 2018-01-15 19:02 GMT+01:00 tomballgithub <notifications@github.com>:

…

@darkelement1987 <https://github.com/darkelement1987> Yes there is another PR that allows you to hide pokemon that are not being notified. So you could set the minimum level and IV to what you want and then 'hide unnotified pokemon' and it will only show the high level ones (excluding the others) #2377 <#2377> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2399 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AUJj-7ng-qMinFhCu4iiIN_ifziV4xxQks5tK5KygaJpZM4REZSf> .

tomballgithub · 2018-01-15T23:16:17Z

Rebased...

Alderon86 · 2018-01-18T17:21:53Z

static/js/map.js

+            if (isNaN(notifiedMinLevel) || notifiedMinLevel <= 0) {
+                notifiedMinLevel = ''
+            }
+            if (notifiedMinLevel > 40) {


Wild pokemon can be max lvl 30 and 35 with weather boost, just a tiny notice.

Using level 40 allows this code to be future proof.

I would remove this check and set the upper limit to 35 in the input in the HTML template. If anyone tampers with it to allow values above 35, the code is right to assume that they only want to be notified of levels at or above whichever level they've set.

There doesn't seem to be a simple way to do this in the template. Min/Max values are only applicable if input type is number not text. Changing to number causes a behavior change in that there are now scroll bars up/down and the input box gets resized. I'll leave it as-is but change to 35 maximum even though I'd prefer 40 to be future proof.

fosJoddie · 2018-01-18T17:33:18Z

static/js/map.common.js

@@ -869,6 +869,9 @@ var StoreOptions = {
    'excludedRarity': {
        default: 0, // 0: none, 1: <=Common, 2: <=Uncommon, 3: <=Rare, 4: <=Very Rare, 5: <=Ultra Rare
        type: StoreTypes.Number
+    'remember_text_level_notify': {


js object literal syntax error, npm run build will fail

Thank you, fixed. Was introduced during rebase.

sebastienvercammen · 2018-01-21T17:47:30Z

static/js/map.js

        hasHighIV = notifiedMinPerfection > 0 && perfection >= notifiedMinPerfection
+        hasHighLevel = notifiedMinLevel > 0 && level >= notifiedMinLevel
+
+        hasHighAttributes = (hasHighIV && !(notifiedMinLevel > 0)) || (hasHighLevel && !(notifiedMinPerfection > 0)) || hasHighLevel && hasHighIV


You can simplify

hasHighAttributes = (hasHighIV && !(notifiedMinLevel > 0)) || (hasHighLevel && !(notifiedMinPerfection > 0)) || hasHighLevel && hasHighIV

to

const shouldNotifyForIV = (hasHighIV && notifiedMinLevel <= 0) const shouldNotifyForLevel = (hasHighLevel && (hasHighIV || notifiedMinPerfection <= 0)) hasHighAttributes = shouldNotifyForIV || shouldNotifyForLevel

Note that I used <= 0 because it's the reverse of >. If < is unnecessary, change it to ==.

This was due to a bug caused by the '' assignment as default

sebastienvercammen · 2018-01-21T17:54:58Z

static/js/map.js

+        $textLevelNotify.on('change', function (e) {
+            notifiedMinLevel = parseInt($textLevelNotify.val(), 10)
+            if (isNaN(notifiedMinLevel) || notifiedMinLevel <= 0) {
+                notifiedMinLevel = ''


The default should be 0 rather than an empty string and the <= 0 should be changed with < 0. 0 can be a valid option, to disable it. Make sure to keep the behavior consistent with "Notify by IV".

I agree but note that I copied the exact '' assignment also used for remember_text_perfection_notify

sebastienvercammen · 2018-01-21T17:58:11Z

static/js/map.js

+            if (isNaN(notifiedMinLevel) || notifiedMinLevel <= 0) {
+                notifiedMinLevel = ''
+            }
+            if (notifiedMinLevel > 40) {


I would remove this check and set the upper limit to 35 in the input in the HTML template. If anyone tampers with it to allow values above 35, the code is right to assume that they only want to be notified of levels at or above whichever level they've set.

Fixed travis Change code to handled undefined notification parameters

tomballgithub · 2018-01-23T05:43:48Z

Should be all rebased and fixed

jaake77

Tested with some IV accounts and it works.
Thanks for the feature!

sebastienvercammen · 2018-01-23T16:09:33Z

static/js/map.js

    }

-    return hasHighIV
+    return hasHighAttributes


The current code will cause a problem when "Notify by IV" is enabled and "Notify by level" is disabled, and the Pokémon has IV data but no cp_multiplier (technically not possible since they're both inserted during scan, but logically they're unrelated so they should be separated).

The problem lies in your condition:

var hasHighAttributes = false if (poke['individual_attack'] != null && poke['cp_multiplier'] !== null) { ... } return hasHighAttributes

tomballgithub · 2018-01-26T15:34:24Z

Approved via code review only, not testing of latest changes

jaake77

Fixed it! ty.

tomballgithub force-pushed the NotifyByLevel branch from 593ec2c to 3268554 Compare December 16, 2017 17:59

FrostTheFox added awaiting review enhancement front-end labels Dec 17, 2017

sLoPPydrive reviewed Dec 19, 2017

View reviewed changes

sirlindqvist approved these changes Dec 23, 2017

View reviewed changes

tomballgithub force-pushed the NotifyByLevel branch from e758135 to 4d683fb Compare January 15, 2018 23:15

Alderon86 reviewed Jan 18, 2018

View reviewed changes

fosJoddie suggested changes Jan 18, 2018

View reviewed changes

sebastienvercammen requested changes Jan 21, 2018

View reviewed changes

Alderon86 approved these changes Jan 21, 2018

View reviewed changes

tomballgithub added 2 commits January 22, 2018 23:19

Add user interface option to Notify By Pokemon Level

ab964b3

Fixed travis Change code to handled undefined notification parameters

Fix typo

5d44772

tomballgithub force-pushed the NotifyByLevel branch from a1f19ee to 5d44772 Compare January 23, 2018 05:19

tomballgithub added 4 commits January 22, 2018 23:20

Fix rebase issue

62d9912

Make changes based on review input

317f365

Cleanup

ac92978

Remove errant const

0f3cb05

jaake77 approved these changes Jan 23, 2018

View reviewed changes

sebastienvercammen requested changes Jan 23, 2018

View reviewed changes

Fix conflict in logic.

5e7a62f

sebastienvercammen approved these changes Jan 26, 2018

View reviewed changes

ClarkyKent approved these changes Jan 26, 2018

View reviewed changes

jaake77 approved these changes Jan 27, 2018

View reviewed changes

sebastienvercammen merged commit 57d4902 into RocketMap:develop Jan 27, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add user interface option to Notify by Pokemon Level #2399

Add user interface option to Notify by Pokemon Level #2399

tomballgithub commented Dec 16, 2017

sLoPPydrive Dec 19, 2017

tomballgithub Dec 19, 2017

tomballgithub commented Dec 19, 2017 •

edited

sirlindqvist left a comment

darkelement1987 commented Jan 15, 2018

tomballgithub commented Jan 15, 2018

darkelement1987 commented Jan 15, 2018 via email

tomballgithub commented Jan 15, 2018

Alderon86 Jan 18, 2018

tomballgithub Jan 19, 2018

sebastienvercammen Jan 21, 2018

tomballgithub Jan 23, 2018

fosJoddie Jan 18, 2018

tomballgithub Jan 19, 2018

sebastienvercammen Jan 21, 2018

tomballgithub Jan 23, 2018

sebastienvercammen Jan 21, 2018

tomballgithub Jan 23, 2018

sebastienvercammen Jan 21, 2018

tomballgithub commented Jan 23, 2018

jaake77 left a comment •

edited

sebastienvercammen Jan 23, 2018

tomballgithub commented Jan 26, 2018

jaake77 left a comment

Add user interface option to Notify by Pokemon Level #2399

Add user interface option to Notify by Pokemon Level #2399

Conversation

tomballgithub commented Dec 16, 2017

Description

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomballgithub commented Dec 19, 2017 • edited

sirlindqvist left a comment

Choose a reason for hiding this comment

darkelement1987 commented Jan 15, 2018

tomballgithub commented Jan 15, 2018

darkelement1987 commented Jan 15, 2018 via email

tomballgithub commented Jan 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomballgithub commented Jan 23, 2018

jaake77 left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomballgithub commented Jan 26, 2018

jaake77 left a comment

Choose a reason for hiding this comment

tomballgithub commented Dec 19, 2017 •

edited

jaake77 left a comment •

edited