edit-inventory-item.html

Clone Tools
  • last updated a few seconds ago
Constraints
Constraints: committers
 
Constraints: files
Constraints: dates
Nick Reid Mateusz Kwiatkowski So what do we want to do? Do want to add the LocalDatabase or leave it as it is?

Nick Reid Mateusz Kwiatkowski So what do we want to do? Do want to add the LocalDatabase or leave it as it is?

OLMIS-3296:: removed facility program filter for inventory item

  1. … 2 more files in changeset.
We will be keeping them in the memory regardless, because we need to parse those permission strings and facilities to get proper set of data. There will never be 1000 programs, just a couple, facil...

We will be keeping them in the memory regardless, because we need to parse those permission strings and facilities to get proper set of data.
There will never be 1000 programs, just a couple, facilities have only id and name (in Malawi there are 700).

Overall this looks great – I do think there is a concern about how these data caches are stored to memory // but I think the search structure has become simpler – and I think it will be hard to imp...

Overall this looks great – I do think there is a concern about how these data caches are stored to memory // but I think the search structure has become simpler – and I think it will be hard to improve more with out running a profiler against it when there is performance data involved.

This does worry me – regardless of how big the dataset really gets.... Questions: (A) How much of this data can we create and save at login? (so it can be quickly loaded) (B) Loading and throwing ...

This does worry me – regardless of how big the dataset really gets....

Questions:
(A) How much of this data can we create and save at login? (so it can be quickly loaded)
(B) Loading and throwing away permissions data is ok // and should be data collected immedately if done in a promise
(C) Does this service really need to be a service? Could it just be a factory that gets instantiated? Ideally that will remove any data created in this screen from memory once the $scope is destroyed

If we can make small-ish data structures, and save them in localStorage (which is a super fast read/write) that would be best...

Overall, I think the best thing to do here, is figure out how to test it against the old version.... and make follow up tickets (IMHO, we have made this a step better, and perfect is something that never really happens)

With this and performance data we're basically keeping 10000 facilities and 1000 programs in the memory (+ the permission strings) is it really a good idea Nick Reid?

With this and performance data we're basically keeping 10000 facilities and 1000 programs in the memory (+ the permission strings) is it really a good idea Nick Reid?

Yea, I wasn't sure what we want to return if module did not register rights to this service.... so I'm just using all, as it was before. I could change it to return empty list if you want.

Yea, I wasn't sure what we want to return if module did not register rights to this service.... so I'm just using all, as it was before. I could change it to return empty list if you want.

Those variables contain all data retrieved from localStorage.... I mean when loadData method is called it loads it here, I'm not sure if it's OK to keep it in memory after starting the page...... I...

Those variables contain all data retrieved from localStorage.... I mean when loadData method is called it loads it here, I'm not sure if it's OK to keep it in memory after starting the page...... I could remake this to call localStorage directly, but the problem is that we will be calling localStorage (later db) on each program change.....

So, I'm a little lost with this review – and am going to get back to it on Sunday I played with the functionality and it seems right – so I'd be happy if this went through QA (assuming nikodem's e...

So, I'm a little lost with this review – and am going to get back to it on Sunday

I played with the functionality and it seems right – so I'd be happy if this went through QA (assuming nikodem's edits get made)

I guess we no longer need the init flag here.

I guess we no longer need the init flag here.

This is just awesome.

This is just awesome.

I think we could pull this line into tests so we can see (especially when testing getSupervisedFacilities) that we're actually testing how the methods interact with each other.

I think we could pull this line into tests so we can see (especially when testing getSupervisedFacilities) that we're actually testing how the methods interact with each other.

Shouldn't we use local storage/local database here?

Shouldn't we use local storage/local database here?

I guess we can clean this a bit, like this;             var userId = authorizationService.getUser().user_id;               return $q.all([                 facilityService.getAllMinimal(),          ...

I guess we can clean this a bit, like this;

            var userId = authorizationService.getUser().user_id;
 
            return $q.all([
                facilityService.getAllMinimal(),
                programService.getUserPrograms(),
                permissionService.load(userId)
            ])
            .then(function(responses) {
                facilities = responses[0];
                programs = responses[1];
                permissions = responses[2];
 
                return referencedataUserService.get(userId);
            })
            .then(function(user) {
                homeFacility = getFacilityById(user.homeFacilityId);
            });


or like this

            var userId = authorizationService.getUser().user_id;
 
            return $q.all([
                facilityService.getAllMinimal(),
                programService.getUserPrograms(),
                permissionService.load(userId)
            ])
            .then(function(responses) {
                facilities = responses[0];
                programs = responses[1];
                permissions = responses[2];
            })
            .then(function() {
                return referencedataUserService.get(userId);
            })
            .then(function(user) {
                homeFacility = getFacilityById(user.homeFacilityId);
            });
*It have to be called

*It have to be called

So if we don't have the right for a specific module, we return rights to all modules? Could you explain the concept here?

So if we don't have the right for a specific module, we return rights to all modules? Could you explain the concept here?

OLMIS-3296: refactored facility program select or use permission strings and cahced minimal...
OLMIS-3296: refactored facility program select or use permission strings and cahced minimal...
Nikodem Graczewski – this looks good I resolved the last issue I had, as it looks like the tests were simplified

Nikodem Graczewski – this looks good

I resolved the last issue I had, as it looks like the tests were simplified

OLMIS-2921: Removed redundant logic tests from html specs

  1. … 4 more files in changeset.
Nikodem Graczewski Thanks for breaking down your concerns a little – You are right that there is a need for automated tests with a scope that is larger than a unit test We definitely don't want to...

Nikodem Graczewski Thanks for breaking down your concerns a little – You are right that there is a need for automated tests with a scope that is larger than a unit test

We definitely don't want to have two unit tests that test the same thing – that violates DRY and orthogonal code

I feel that HTML and HTML unit tests should check for the appearance of specific strings/elements, and that methods/events defined in the form call the right function // I'd be concerned if they had a larger scope than that

We do need to build requirements around UI integration tests, as there is weird behavior there – and the only way we can prove we are fixing are integration tests

anyways – Yes, please remove duplicative tests

cool — I feel like positive configurations are easier to reason about.... but I'm not worried about this too much ie "trackable:false"

cool — I feel like positive configurations are easier to reason about.... but I'm not worried about this too much

ie "trackable:false"

https://openlmis.atlassian.net/browse/?focusedCommentId=44322&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#OLMIS-2921comment-44322

https://openlmis.atlassian.net/browse/?focusedCommentId=44322&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#OLMIS-2921comment-44322

Nick Reid I guess a large chunk of the code tested here (and in other html tests) is in the controller: *clearing reason selection after status change *clearing decommission date after status ch...

Nick Reid

I guess a large chunk of the code tested here (and in other html tests) is in the controller:

  • clearing reason selection after status change
  • clearing decommission date after status change
  • cancel button opening confirmService (basically all cancel button tests)
  • calling inventoryItemService after submitting form


Should I remove those too? I always though about those tests as a way of testing the whole page integrity.

Nick Reid It's our custom property for (not) tracking the state with stateTracker.

Nick Reid It's our custom property for (not) tracking the state with stateTracker.

Yea... but the code that actually does that work is the controller.... If we were setting that logic in the html (like a form action parameter) then this would make sense here If the HTML doesn't ...

Yea... but the code that actually does that work is the controller.... If we were setting that logic in the html (like a form action parameter) then this would make sense here

If the HTML doesn't know about it, then the HTML shouldn't test it