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

Feature/ 'assessments levels over the years' chart #80

Merged
merged 12 commits into from
Oct 28, 2019

Conversation

kowal
Copy link
Contributor

@kowal kowal commented Oct 14, 2019

Summary

Add Assessment Levels/years chart for (TPI) Management Quality for Company

Details

  • New endpoint returns data in format: [[2016,2],[2017,2],[2018,4]]
  • ::Api::Charts::Company.new(@company).nr_of_assessments_data always provides data for each year starting from latest year to most recent, filling missing years levels with last reported level.

Mockups

tpi-mocks

Example

More than 1 assessment (http://localhost:3000/tpi/companies/314)

tpi-levels

When only 1 assessment (http://localhost:3000/tpi/companies/460)

tpi-2

Questions

  • should the chart X axis end on last reported year or current year? Yes
  • what to do with companies which has only single MQ assessment in current year? We can't draw line in that case, so chart doesn't make sense. Draw a dot

@kowal kowal requested review from tsubik and simaob October 14, 2019 12:38
@simaob
Copy link
Contributor

simaob commented Oct 14, 2019

Thanks for this Kowal! =) I think that we should show current year as the latest year on the xx axis, this way it gives more context and can make it easier for people to see if companies submitted something this year.

One there's only one MQ Assessment we can just show a single dot, no? I think it's fair.

@kowal
Copy link
Contributor Author

kowal commented Oct 14, 2019

.. I think that we should show current year as the latest year on the xx axis, this way it gives more context and can make it easier for people to see if companies submitted something this year.

Yep, make sense, so I need to fix that in this PR

One there's only one MQ Assessment we can just show a single dot, no? I think it's fair.

OK, that's how it works now.

@kowal
Copy link
Contributor Author

kowal commented Oct 14, 2019

.. I think that we should show current year as the latest year on the xx axis, this way it gives more context and can make it easier for people to see if companies submitted something this year.

Yep, make sense, so I need to fix that in this PR

.. although it might make sense to somehow distinguish it visually. This case means that we don't have data for those last year(s), but we're assuming MQ a. level didn't change - but it could - a company could have done better/worst job but we just don't know it.

@kowal
Copy link
Contributor Author

kowal commented Oct 14, 2019

@simaob I fixed the chart to always end on the current year, also I simplified the logic. I still need to add tests for this.

Copy link
Contributor

@tsubik tsubik left a comment

Choose a reason for hiding this comment

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

I wonder if we should take a year from an assessment date or publication date. Also what happens when there are two assessments/publications in the given year. Take a look at company Bukit Asam, the level is 1 because in the same year there were two assessments. But the second assessment was published the next year.

image

image

Maybe we should take publication date for this chart.

app/controllers/tpi/companies_controller.rb Outdated Show resolved Hide resolved
app/services/api/presenters/company.rb Outdated Show resolved Hide resolved
@kowal kowal changed the title Feature/no of assessments chart Feature/ 'assessments levels over the years' chart Oct 15, 2019
@kowal
Copy link
Contributor Author

kowal commented Oct 15, 2019

I wonder if we should take a year from an assessment date or publication date. Also what happens when there are two assessments/publications in the given year. Take a look at company Bukit Asam, the level is 1 because in the same year there were two assessments. But the second assessment was published the next year.

Maybe we should take publication date for this chart.

Great catch! will fix that in the next days. I think switching to publication date make sense. @simaob ?

@tsubik
Copy link
Contributor

tsubik commented Oct 15, 2019

Or we should just go more granular there. Not years, but month-year for axis-x

@tsubik
Copy link
Contributor

tsubik commented Oct 15, 2019

There could be 2 publications within one year.

@faustoperez
Copy link
Contributor

Hi folks! The assessment dates should have a month and a year, so more granularity is needed.

On the x-axis, we would show the year only, but we could have an additional data point if needed.

It's covered on the design mockups (2 data points for March 2018)

image

Hope this helps :)

@kowal kowal force-pushed the feature/no-of-assessments-chart branch from ecd88b4 to bbca9b5 Compare October 21, 2019 19:24
@tsubik
Copy link
Contributor

tsubik commented Oct 28, 2019

I only show the points for real assessments. I removed that assumption that level stays the same when there was no assessment in the given year.

image

So the chart looks like this now. The number of data points is the number of assessments.

Do you think that is a good approach @simaob ?

@tsubik
Copy link
Contributor

tsubik commented Oct 28, 2019

Also, I'm using slugs to access companies and sectors on the public site. http://localhost:3000/tpi/companies/acerinox, http://localhost:3000/tpi/companies/bukit-asam

Copy link
Contributor

@tsubik tsubik left a comment

Choose a reason for hiding this comment

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

👍

@tsubik tsubik merged commit 9645f24 into develop Oct 28, 2019
@simaob simaob deleted the feature/no-of-assessments-chart branch October 30, 2019 10:22
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