Skip to content

Hardcoded statuses 72#101

Closed
cazgp wants to merge 3 commits intomasterfrom
hardcoded_statuses_72
Closed

Hardcoded statuses 72#101
cazgp wants to merge 3 commits intomasterfrom
hardcoded_statuses_72

Conversation

@cazgp
Copy link
Copy Markdown
Member

@cazgp cazgp commented Aug 24, 2014

No description provided.

@kynan kynan force-pushed the hardcoded_statuses_72 branch from 21d2232 to 18e05c1 Compare August 24, 2014 15:19
@kynan
Copy link
Copy Markdown
Member

kynan commented Aug 24, 2014

Have rebased the branch. This looks like a good start, wondering if we should wait until we know how to deal with fuzziness.

@cazgp
Copy link
Copy Markdown
Member Author

cazgp commented Aug 25, 2014

I think part of the problem is that a lot of the display code depends upon those statuses being named. If we generalise the display code and ensure that statuses are always passed in the order working -> however many in betweens -> completely borked, then we never even have to look at the names of the statuses.

I'm not even sure that fuzziness is needed. If a user can only ever input the statuses (and a pre-parse of data imported from elsewhere handles the fuzziness), the scripts can be quite simple. Or tis there something fundamental I'm missing?

@kynan
Copy link
Copy Markdown
Member

kynan commented Aug 25, 2014

Can you expand on how we could simplify the display code that way?

I agree that fuzziness is a separate issue that needs to be handled elsewhere.

@dgorissen
Copy link
Copy Markdown
Member

1st point: Have not had the chance yet to look into the code & changes in detail so can't really answer that yet. Note it is not inconceivable we move to a hierarchical status model in the future instead of a linear one.

2nd point: yeah, fuzziness really only comes into play on data import or when reporting by sms. So should be another issue really.

@cazgp
Copy link
Copy Markdown
Member Author

cazgp commented Aug 25, 2014

Sure, throughout the graphing stuff is things like:

.group(group, "Functional")
...
.stack(group, "Non-Functional")
.stack(group, "Needs Repair")

However, it could be something like:

graph = graph.group(group, statuses[0].displayName)
for status in statuses.slice(1)
    graph = graph.stack(group, status.displayName)

There's also a lot of stuff like

if something.functional
    do something
else if something["needs repair"]
    do something else

With a guaranteed ordered list that's:

if something[statuses[0]]
    do something
else if something[statuses[-1]]
    do something else

I've yet to see anything between the end points being checked so I could be wrong.

@kynan kynan force-pushed the hardcoded_statuses_72 branch from 18e05c1 to d919321 Compare August 26, 2014 06:37
@kynan
Copy link
Copy Markdown
Member

kynan commented Aug 26, 2014

Rebased again, please check carefully, there were lots of tricky merge conflicts.

Comment thread app/scripts/services.coffee Outdated
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This should be plural... $rootScope.waterpointStatuses...

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I've renamed the service

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yeah but this is adding it to scope, so you can define it however you want. There are multiple statuses available so it should be statuses :)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ah, yes, replace fail.

@kynan kynan force-pushed the hardcoded_statuses_72 branch from d919321 to e1afd7d Compare August 26, 2014 06:47
@cazgp
Copy link
Copy Markdown
Member Author

cazgp commented Aug 26, 2014

This looks good to me :) What do you think about the linear progression so's we can remove all references to statuses?

@kynan
Copy link
Copy Markdown
Member

kynan commented Aug 26, 2014

Not sure, that's the dashboard i.e. @dgorissen's territory.

Caroline Nadel and others added 3 commits August 26, 2014 11:50
* Add a new factory to services.coffee which gets statuses from
  the Flask taarifa_waterpoints.py
* Allow statuses to be globally accessible through the $rootScope
  so templates now use that instead of hard-coded.
* Convert the map generation into using promises so the statuses
  can be non-hard-coded.
* Things left I have no idea about:
  * plots.js is a bunch of global functions which isn't used by
    angular.
  * There are some places in the code which checks is status
    functional? Need some input on how to do that - status ID or
    something maybe?
  * Nothing is fuzzy yet.
@kynan kynan force-pushed the hardcoded_statuses_72 branch from e1afd7d to 6bc830b Compare August 26, 2014 10:59
@kynan
Copy link
Copy Markdown
Member

kynan commented Aug 26, 2014

Have rebased again, but this will need updating w.r.t. status -> cost mapping added in @dgorissen's heatmap pr #128 (i.e. these are still hard coded).

@kynan
Copy link
Copy Markdown
Member

kynan commented Oct 13, 2014

This has been rotting for a while. Do we still want this @dgorissen? If so, should clean up and merge to prevent even further bitrot.

@dgorissen
Copy link
Copy Markdown
Member

Same comment as #130 .. needs time :( Would already be good if we could at least merge what was done so far

@kynan
Copy link
Copy Markdown
Member

kynan commented Oct 13, 2014

Yes, maybe @cazgp can spring into action again and update this pr?

@cazgp
Copy link
Copy Markdown
Member Author

cazgp commented Oct 14, 2014

I'm not sure what else I needed to do on this? I thought it was good to go but then it wasn't. Unfortunately I don't have so much coding time atm.

@kynan
Copy link
Copy Markdown
Member

kynan commented Oct 14, 2014

Don't remember exactly, I think we were waiting on feedback from @dgorissen regarding dashboard-related changes. For starters this would need rebasing. Might get to it at some point, but can't promise.

@cazgp cazgp closed this Mar 12, 2020
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.

3 participants