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

Learner dashboard project milestone 2.1 #3591

Merged
merged 26 commits into from Jul 18, 2017

Conversation

Projects
None yet
4 participants
@Arunabh98
Copy link
Contributor

commented Jul 1, 2017

Milestone 2.1 of the learner dashboard project. The details of this milestone can be found over here.

@Arunabh98 Arunabh98 requested review from seanlip, jaredsilver and rachelwchen Jul 1, 2017

@codecov-io

This comment has been minimized.

Copy link

commented Jul 1, 2017

Codecov Report

Merging #3591 into develop will decrease coverage by 0.4%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           develop   #3591      +/-   ##
==========================================
- Coverage    45.21%   44.8%   -0.41%     
==========================================
  Files          254     257       +3     
  Lines        19528   19902     +374     
  Branches      3092    3136      +44     
==========================================
+ Hits          8829    8917      +88     
- Misses       10699   10985     +286
Impacted Files Coverage Δ
.../dev/head/domain/exploration/StateObjectFactory.js 86.66% <0%> (-2.23%) ⬇️
...ead/pages/exploration_player/TutorCardDirective.js 2.98% <0%> (-0.94%) ⬇️
...es/exploration_player/SupplementalCardDirective.js 1.96% <0%> (-0.9%) ⬇️
extensions/objects/templates/FilepathEditor.js 0.26% <0%> (-0.45%) ⬇️
.../exploration_editor/editor_tab/StateInteraction.js 33.51% <0%> (-0.36%) ⬇️
...tivities_tab/AdminDevModeActivitiesTabDirective.js 1.35% <0%> (-0.32%) ⬇️
...ploration_editor/feedback_tab/ThreadDataService.js 25% <0%> (-0.28%) ⬇️
.../pages/exploration_editor/editor_tab/StateHints.js 1.49% <0%> (-0.18%) ⬇️
...ges/exploration_editor/settings_tab/SettingsTab.js 0.61% <0%> (-0.01%) ⬇️
...pages/exploration_editor/history_tab/HistoryTab.js 1.02% <0%> (ø) ⬆️
... and 17 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f33f569...34f3e78. Read the comment docs.

which matches with the given user_ids and exploration_id.
"""
instance_ids = [cls.generate_full_id(user_id, exploration_id, thread_id)
for exploration_id, thread_id in zip(

This comment has been minimized.

Copy link
@jaredsilver

jaredsilver Jul 2, 2017

Contributor

Is the indentation right here?

This comment has been minimized.

Copy link
@Arunabh98

Arunabh98 Jul 3, 2017

Author Contributor

Yep. This is right according to pylint.

@jaredsilver

This comment has been minimized.

Copy link
Contributor

commented Jul 2, 2017

Question: the way this is currently architected, we're storing every message ID that a user has read in the FeedbackThreadUserModel, right? Would it be better to store only the ID of the latest one they've read? Then, every one before that (by created_on) would just be considered read? I don't really know the answer to this, but it was just a thought that came to mind. (Also, I have very little experience with NoSQL datastores, so I might just not be realizing that this is standard practice the way it currently is.)

@jaredsilver

This comment has been minimized.

Copy link
Contributor

commented Jul 2, 2017

Aside from that, I think this looks good to me.

@jaredsilver

This comment has been minimized.

Copy link
Contributor

commented Jul 2, 2017

Ah, one other thing: is get_exploration_titles_by_ids in here?

@seanlip
Copy link
Member

left a comment

Hi @Arunabh98, looks like a good start -- I've done a quick first pass and left a few notes.

This PR needs two things though:

(a) A full backend integration test with actors Alice, Bob, Charlie who are all creating explorations and passing messages around. Try to simulate a real-life situation and walk through the various possible interactions. This ensures the whole thing works end-to-end.

(b) A backend load test. Create 100 or 1000 threads with maybe 100 messages each, and load the learner dashboard. Time it. If it's unreasonably long, impose limits on e.g. how many second-last messages you're going to show. This will help you make sure that appropriate limits are in place and that the learner dashboard doesn't break in production.

Thanks!

messages = [m.to_dict() for m in feedback_services.get_messages(
exploration_id, thread_id)]
message_ids = [message['message_id'] for message in messages]
feedback_services.update_messages_read_by_the_user(

This comment has been minimized.

Copy link
@seanlip

seanlip Jul 2, 2017

Member

Why are you doing this at GET time? GET actions, in general, should not change the datastore. Shouldn't the "marking as read" be done when a message is opened?

This comment has been minimized.

Copy link
@seanlip

seanlip Jul 2, 2017

Member

Oh -- maybe I misunderstand. Is this supposed to be update_messages_shown_to_the_user()? ("Shown to" is not the same as "read by".)

That said, isn't "read by" what you actually care about here?

This comment has been minimized.

Copy link
@seanlip

seanlip Jul 11, 2017

Member

@Arunabh98 -- you haven't responded to this?

This comment has been minimized.

Copy link
@seanlip

seanlip Jul 11, 2017

Member

Actually, thinking about it more, I think what you're doing is probably fine, since there isn't the possibility for viewing individual messages. So I guess it's the best that can be done!

This comment has been minimized.

Copy link
@Arunabh98

Arunabh98 Jul 11, 2017

Author Contributor

Yes that was my thought too.

if self.user_id is None:
raise self.PageNotFoundException

messages_object = feedback_services.get_messages(

This comment has been minimized.

Copy link
@seanlip

seanlip Jul 2, 2017

Member

messages_object --> messages (it's just a list, right? messages_object kind of suggests an instance of a Messages class)

exploration_id: str. The id of the exploration.
thread_id. str. The id of the thread.
user_id: str. The id of the user reading the messages,
message_ids: list(int): The ids of the messages to be added to the read

This comment has been minimized.

Copy link
@seanlip

seanlip Jul 2, 2017

Member

In the implementation it seems like you are replacing this list, not adding to it...

This comment has been minimized.

Copy link
@Arunabh98

Arunabh98 Jul 4, 2017

Author Contributor

Fixed.

This comment has been minimized.

Copy link
@seanlip

seanlip Jul 11, 2017

Member

Doesn't look like it's fixed. Docstring and code should be consistent.

list(dict). A list of dictionary containing the summaries of the threads
given to it.
"""
exploration_ids = [thread_id.split('.')[0] for thread_id in full_thread_ids]

This comment has been minimized.

Copy link
@seanlip
user_id, strict=False)

return (
subscriptions_model.feedback_thread_ids

This comment has been minimized.

Copy link
@seanlip

seanlip Jul 2, 2017

Member

Just to check, does this include suggestion thread ids as well? Should it?

This comment has been minimized.

Copy link
@Arunabh98

Arunabh98 Jul 4, 2017

Author Contributor

Yes, it does. It should because a suggestion is also kind of feedback only.

This comment has been minimized.

Copy link
@seanlip

seanlip Jul 11, 2017

Member

Ah ok, thanks. Maybe say "all the feedback and suggestion threads" in the docstring, then -- that at least helps guard a bit against the misnaming here (which is hard to change).

This comment has been minimized.

Copy link
@Arunabh98

Arunabh98 Jul 12, 2017

Author Contributor

Done!


messages = []

for full_thread_id in full_thread_ids:

This comment has been minimized.

Copy link
@seanlip

seanlip Jul 2, 2017

Member

Having a query in a for loop is expensive. This isn't going to scale.

Can we limit the length of full_thread_ids?

This comment has been minimized.

Copy link
@Arunabh98

Arunabh98 Jul 4, 2017

Author Contributor

We can limit this to about 100 feedback threads. If we approximate that each thread will have about 5 messages (because first, it would have some feedback by the learner, then the creator will ask for some clarification, the learner clears it up and then the creator fixes the issue) then it takes about 5 seconds to fetch 100 threads having 5 messages. I have checked this in a test that I have included in this PR and I have kept the threshold at 8 seconds for safety purposes.

So would a limit of 100 do? We would choose the 100 based on last updated.

This comment has been minimized.

Copy link
@seanlip

seanlip Jul 4, 2017

Member

You know, one thing that would speed this up is to just maintain a model that keeps track of the last two messages for each thread. Assuming that messages can't be deleted, this auxiliary model can be updated alongside the main model each time a new message comes in. Then you can just get by ID on the auxiliary model.

Initially, the auxiliary model would be populated by a one-off MR job.

Thoughts?

ExplorationUserDataModel. The ExplorationUserDataModel instance
which matches with the given user_ids and exploration_id.
"""
instance_ids = [cls.generate_full_id(user_id, exploration_id, thread_id)

This comment has been minimized.

Copy link
@seanlip

seanlip Jul 2, 2017

Member

Better to break after '['.

This comment has been minimized.

Copy link
@Arunabh98

Arunabh98 Jul 4, 2017

Author Contributor

Done!

feconf.py Outdated
@@ -549,6 +549,7 @@ def get_empty_ratings():
FRACTIONS_LANDING_PAGE_URL = '/fractions'
LEARNER_DASHBOARD_URL = '/learner_dashboard'
LEARNER_DASHBOARD_DATA_URL = '/learnerdashboardhandler/data'
LEARNER_DASHBOARD_FEEDBACK_THREAD_URL = '/learnerdashboardthread'

This comment has been minimized.

Copy link
@seanlip

seanlip Jul 2, 2017

Member

This is a json handler, right? By convention it should end with DATA_URL and the url should end with 'handler'.

This comment has been minimized.

Copy link
@Arunabh98

Arunabh98 Jul 4, 2017

Author Contributor

Done!

@@ -400,6 +401,33 @@ def test_get_thread_summaries(self):
# Check if the summaries match.
self.assertEqual(thread_summary, thread_summaries[index])

def test_get_thread_summaries_load_test(self):
# The speed of fetching the summaries of 100 threads having 5 messages
# should be less than 8 seconds.

This comment has been minimized.

Copy link
@seanlip

seanlip Jul 4, 2017

Member

8 seconds is a long time. It needs to be more like 1.

(Consider adding limits if 100 / 5 is too much. One way to handle this sort of thing is to load data progressively as it is paged to.)

This comment has been minimized.

Copy link
@Arunabh98

Arunabh98 Jul 4, 2017

Author Contributor

40 threads having 5 messages comes under 1 second. 40 also seems like a good number. So should we fetch 40 threads?

This comment has been minimized.

Copy link
@seanlip

seanlip Jul 4, 2017

Member

Yes, that seems reasonable. Try to get it to 0.5 secs, because this is measuring just the query execution time. There's going to be additional overhead from the learner's perspective in terms of the round-trip time to the server, additional backend latency from other sources, etc.

This comment has been minimized.

Copy link
@seanlip

seanlip Jul 4, 2017

Member

Also, you don't want to be "brushing up" against the 1 second limit. It should be comfortably under. Otherwise what you're gonna end up with is flaky backend tests.

This comment has been minimized.

Copy link
@Arunabh98

Arunabh98 Jul 4, 2017

Author Contributor

Okay, I'll try to see where I can optimize.


@staticmethod
def map(item):
yield (item.thread_id, 1)

This comment has been minimized.

Copy link
@seanlip

seanlip Jul 5, 2017

Member

I'm not 100% sure we haven't deleted any messages. It might be safer to yield the message ID and pick the number that's one more than the max message ID to assign as the message count. In reduce(), also yield an error message (with the exp id and thread id and thread subject) if this number does not equal the number of messages in the list -- we can then run this when testing to verify.

This comment has been minimized.

Copy link
@Arunabh98

Arunabh98 Jul 6, 2017

Author Contributor

Done!

user_id, exploration_id, thread_id)
for exploration_id, thread_id in zip(exploration_ids, thread_ids)])

last_two_messages_ids = (

This comment has been minimized.

Copy link
@seanlip

seanlip Jul 11, 2017

Member

You're going to retrieve the thread models anyway. Why not retrieve them and convert to domain objects after generating thread_model_ids (above), and then obtain the last_two_messages_ids directly from there (you can define a helper function in the FeedbackThread domain object)? Then you don't need to get the thread models twice.

This comment has been minimized.

Copy link
@Arunabh98

Arunabh98 Jul 12, 2017

Author Contributor

Nice idea. Done!

This comment has been minimized.

Copy link
@seanlip

seanlip Jul 13, 2017

Member

Doesn't look done...

thread_summaries = []
for index, model in enumerate(thread_models):

does_second_message_exist = (last_two_messages[index][1] is not None)

This comment has been minimized.

Copy link
@seanlip

seanlip Jul 11, 2017

Member

Nit: might as well put this directly above line 512 so that there's less context that the reader needs to remember.

This comment has been minimized.

Copy link
@Arunabh98

Arunabh98 Jul 12, 2017

Author Contributor

Done!

last_two_messages = [messages[i:i + 2] for i in range(0, len(messages), 2)]

thread_summaries = []
for index, model in enumerate(thread_models):

This comment has been minimized.

Copy link
@seanlip

seanlip Jul 11, 2017

Member

Try to use the thread domain objects here. Better not to do too much logic with the raw models.

This comment has been minimized.

Copy link
@Arunabh98

Arunabh98 Jul 12, 2017

Author Contributor

Used the domain object.



class FeedbackThreadMessagesCountOneOffJob(jobs.BaseMapReduceJobManager):
"""One-off job for calculating the distribution of username lengths."""

This comment has been minimized.

Copy link
@seanlip

This comment has been minimized.

Copy link
@Arunabh98

Arunabh98 Jul 12, 2017

Author Contributor

Fixed.

feedback_services.set_message_count_for_thread(
key, max(message_ids) + 1)
else:
logging.error('The number of messages estimated by message ids is' +

This comment has been minimized.

Copy link
@seanlip

seanlip Jul 11, 2017

Member

This error message needs more info about the thread ID, observed number, expected number, etc. Otherwise it's hard to debug.

Also, there should be a space char after 'is'.

This comment has been minimized.

Copy link
@Arunabh98

Arunabh98 Jul 12, 2017

Author Contributor

Fixed.


@classmethod
def get_multi(cls, user_id, exploration_ids, thread_ids):
"""Gets the ExplorationUserDataModel for the given user and exploration

This comment has been minimized.

Copy link
@seanlip

seanlip Jul 11, 2017

Member

Docstring is totally wrong.

This comment has been minimized.

Copy link
@Arunabh98

Arunabh98 Jul 12, 2017

Author Contributor

Fixed.


@classmethod
def get(cls, user_id, exploration_id, thread_id):
"""Gets the FeedbackThreadUserModel for the given user and thread id.

This comment has been minimized.

Copy link
@seanlip

seanlip Jul 11, 2017

Member

Docstring is wrong.

This comment has been minimized.

Copy link
@Arunabh98

Arunabh98 Jul 12, 2017

Author Contributor

Fixed.

message_ids_read_by_user = ndb.IntegerProperty(repeated=True, indexed=True)

@classmethod
def generate_full_id(cls, user_id, exploration_id, thread_id):

This comment has been minimized.

Copy link
@seanlip

seanlip Jul 11, 2017

Member

Should this be a private method (with leading underscore)?

This comment has been minimized.

Copy link
@Arunabh98

Arunabh98 Jul 12, 2017

Author Contributor

I did not keep this a private method as I would need it to generate ids outside the class. This is similar to the 'generate_full_thread_id' method of FeedbackThreadModel.

This comment has been minimized.

Copy link
@seanlip

seanlip Jul 12, 2017

Member

OK, thanks for explaining.

return '%s.%s' % (user_id, full_thread_id)

@classmethod
def get(cls, user_id, exploration_id, thread_id):

This comment has been minimized.

Copy link
@seanlip

seanlip Jul 11, 2017

Member

Please add tests for all publicly-exposed methods.

This comment has been minimized.

Copy link
@Arunabh98

Arunabh98 Jul 12, 2017

Author Contributor

Added tests for all publicly-exposed methods.

index.yaml Outdated
@@ -101,6 +101,19 @@ indexes:
- name: last_updated
direction: desc

- kind: FeedbackMessageModel

This comment has been minimized.

Copy link
@seanlip

seanlip Jul 11, 2017

Member

I'm not sure if these were generated automatically when you had queries last time. If they were, and you don't need these indexes any more, could you please remove them?

This comment has been minimized.

Copy link
@Arunabh98

Arunabh98 Jul 12, 2017

Author Contributor

Removed.

Arunabh98 added some commits Jul 12, 2017

rights_manager.create_new_exploration_rights(
self.EXP_ID, self.owner_id_2)

def get_messages_read_by_user(self, user_id, exploration_id, thread_id):

This comment has been minimized.

Copy link
@seanlip

seanlip Jul 13, 2017

Member

Add leading underscore?

This comment has been minimized.

Copy link
@Arunabh98

Arunabh98 Jul 14, 2017

Author Contributor

Done!

feedback_thread_user_model.message_ids_read_by_user
if feedback_thread_user_model else [])

def get_message_ids_in_a_thread(self, exploration_id, thread_id):

This comment has been minimized.

Copy link
@seanlip

seanlip Jul 13, 2017

Member

Ditto.

This comment has been minimized.

Copy link
@Arunabh98

Arunabh98 Jul 14, 2017

Author Contributor

Done!


message_ids = [m.message_id for m in messages]
feedback_services.update_messages_read_by_the_user(
exploration_id, thread_id, self.user_id, message_ids)

This comment has been minimized.

Copy link
@seanlip

seanlip Jul 13, 2017

Member

Style nit: Odd that self.user_id is nestled between the other args. Maybe put it at the front? It seems like exp_id, thread_id and message_ids should all come together.

This comment has been minimized.

Copy link
@Arunabh98

Arunabh98 Jul 14, 2017

Author Contributor

Done!

"""Returns the full message ids of the last two messages of the thread.
Returns:
list(str). The ids of the last two messages of the thread.

This comment has been minimized.

Copy link
@seanlip

seanlip Jul 13, 2017

Member

There should be defined behavior here for what happens if the thread has < 2 messages, and it should be tested.

This comment has been minimized.

Copy link
@Arunabh98

Arunabh98 Jul 14, 2017

Author Contributor

Done!

This comment has been minimized.

Copy link
@seanlip

seanlip Jul 14, 2017

Member

The docstring should be updated, too. It should fully describe the interface to the method.

exploration_id = exploration_and_thread_id[0]
thread_id = exploration_and_thread_id[1]
thread = feedback_services.get_thread(exploration_id, thread_id)
logging.error('The number of messages in thread, given by the ' +

This comment has been minimized.

Copy link
@seanlip

seanlip Jul 13, 2017

Member

Break after '(' (so you have more room).

Use interpolation: 'The number of messages in the thread with ID %s is %s. But ...' % (...). Easier to read that way.

This comment has been minimized.

Copy link
@Arunabh98

Arunabh98 Jul 14, 2017

Author Contributor

Done!

threads given to it. The format of the returned value:
[
{
'status': Status of the thread,

This comment has been minimized.

Copy link
@seanlip

seanlip Jul 13, 2017

Member

Add type info after each key, e.g. 'status': str. The status of the thread. End each line with a period.

This comment has been minimized.

Copy link
@Arunabh98

Arunabh98 Jul 14, 2017

Author Contributor

Done!

Returns:
list(dict). A list of dictionaries containing the summaries of the
threads given to it. The format of the returned value:

This comment has been minimized.

Copy link
@seanlip

seanlip Jul 13, 2017

Member
threads given to it. Each dict has the following keys:
- 'status': ...

This comment has been minimized.

Copy link
@Arunabh98

Arunabh98 Jul 14, 2017

Author Contributor

Done!

]
"""
exploration_and_thread_ids = (
[thread_id.split('.') for thread_id in full_thread_ids])

This comment has been minimized.

Copy link
@seanlip

seanlip Jul 13, 2017

Member

Declare a function in feedback_models.FeedbackThreadModel to do the conversion instead of doing it here -- it's a counterpart to generate_full_thread_id(). Add tests for it.

This comment has been minimized.

Copy link
@Arunabh98

Arunabh98 Jul 14, 2017

Author Contributor

Done!

user_id, exploration_id, thread_id)
for exploration_id, thread_id in zip(exploration_ids, thread_ids)])

last_two_messages_ids = (

This comment has been minimized.

Copy link
@seanlip

seanlip Jul 13, 2017

Member

Doesn't look done...

[FeedbackThreadModel.generate_full_thread_id(
exploration_id, thread_id)
for exploration_id, thread_id in zip(exploration_ids, thread_ids)])
thread_models = FeedbackThreadModel.get_multi(full_thread_ids)

This comment has been minimized.

Copy link
@seanlip

seanlip Jul 13, 2017

Member

Doesn't look removed. I still see it.

@Arunabh98

This comment has been minimized.

Copy link
Contributor Author

commented Jul 14, 2017

Now I am fetching the threads only once. I have changed the structure accordingly.

@seanlip
Copy link
Member

left a comment

This is close to being done, I think. Just a few more comments!

"""Returns the full message ids of the last two messages of the thread.
Returns:
list(str). The ids of the last two messages of the thread.

This comment has been minimized.

Copy link
@seanlip

seanlip Jul 14, 2017

Member

The docstring should be updated, too. It should fully describe the interface to the method.

@@ -107,6 +109,37 @@ def get_thread_id_from_full_thread_id(full_thread_id):
"""
return full_thread_id.split('.')[1]

def get_full_message_id(self, message_id):
""" Returns the full id of the message.

This comment has been minimized.

Copy link
@seanlip

seanlip Jul 14, 2017

Member

Nit: drop space after """

This comment has been minimized.

Copy link
@Arunabh98

Arunabh98 Jul 15, 2017

Author Contributor

Done.

else:
# The second last message does not exist. So we set it's id to -1
# implying that it doesn't exist.
second_last_message_id = -1

This comment has been minimized.

Copy link
@seanlip

seanlip Jul 14, 2017

Member

No, this is really iffy. You shouldn't be constructing full message IDs from message IDs that are invalid. At this point, if you are appending None, then just do so here rather than exposing the "badness" to other methods.

This comment has been minimized.

Copy link
@Arunabh98

Arunabh98 Jul 15, 2017

Author Contributor

The problem with appending None is that when I try to fetch the models using get_multi ie returns an error if it receives a None argument. What should I do?

This comment has been minimized.

Copy link
@seanlip

seanlip Jul 15, 2017

Member

Can you return a 1-element list?

This comment has been minimized.

Copy link
@Arunabh98

Arunabh98 Jul 15, 2017

Author Contributor

Later after I fetch the models I would have to remember which list had only one element and which had two elements so that I can appropriately split the list. Don't you think this would add more complications?

This comment has been minimized.

Copy link
@seanlip

seanlip Jul 15, 2017

Member

Yes, and it's worth trying to find ways to avoid those complications, but passing invalid message IDs around is not the way to do it.

One possibility is to modify the contract of the function in gae_datastore_services.py so that it accepts None elements in the list of IDs, and deals with them appropriately. I think that would be fine.

This comment has been minimized.

Copy link
@Arunabh98

Arunabh98 Jul 15, 2017

Author Contributor

Modified the contract in gae_models.py. Now it accepts None arguments and just returns None.

u'open', u'a subject', None, False, 1, fake_date, fake_date)

last_two_message_ids = thread_1.get_last_two_message_ids()
# The second last message should be given an id of -1 as it doesn't

This comment has been minimized.

Copy link
@seanlip

seanlip Jul 14, 2017

Member

Again I don't believe this is desired behaviour. It should be possible for a dev to assume that something that's called a message ID is indeed a valid message ID.

This comment has been minimized.

Copy link
@Arunabh98

Arunabh98 Jul 15, 2017

Author Contributor

Now I am returning None only.

thread_id = exploration_and_thread_id[1]
thread = feedback_services.get_thread(exploration_id, thread_id)
logging.error(
'The number of messages in the thread, given by the id ' +

This comment has been minimized.

Copy link
@seanlip

seanlip Jul 14, 2017

Member

Nope -- please do one big string and interpolate the whole thing at the end.

Note that for Python strings you can omit the "+"s at the end of the line. They're automatically concatenated.

This comment has been minimized.

Copy link
@Arunabh98

Arunabh98 Jul 15, 2017

Author Contributor

Done.


last_two_messages_ids = []
for thread in threads:
last_two_messages_ids = (

This comment has been minimized.

Copy link
@seanlip

seanlip Jul 14, 2017

Member

+= might be cleaner here?

Maybe itertools.chain() is helpful too, not sure.

This comment has been minimized.

Copy link
@Arunabh98

Arunabh98 Jul 15, 2017

Author Contributor

Done.


if thread.message_count:
total_no_of_messages = thread.message_count
else:

This comment has been minimized.

Copy link
@seanlip

seanlip Jul 14, 2017

Member

Did we resolve the "add TODO to remove the 'else' bit once we're sure that every thread has a message_count" comment? This is a long function so anything we can do to simplify the code is good.

This comment has been minimized.

Copy link
@seanlip

seanlip Jul 14, 2017

Member

(I'm having trouble finding the original comment, but I'm pretty sure we discussed this.)

This comment has been minimized.

Copy link
@Arunabh98

Arunabh98 Jul 15, 2017

Author Contributor

Added TODO.

@@ -177,7 +177,7 @@ def _run_one_off_job(self):
for stringified_distribution in stringified_output:
value = re.findall(r'\d+', stringified_distribution)
# output['username length'] = number of users
output[value[0]] = int(value[1])
print "Hello", value

This comment has been minimized.

Copy link
@seanlip

This comment has been minimized.

Copy link
@Arunabh98

Arunabh98 Jul 15, 2017

Author Contributor

Removed.

index.yaml Outdated
@@ -101,6 +101,13 @@ indexes:
- name: last_updated
direction: desc

- kind: FeedbackMessageModel

This comment has been minimized.

Copy link
@seanlip

seanlip Jul 14, 2017

Member

Just to verify: is this one still needed? What uses it?

This comment has been minimized.

Copy link
@Arunabh98

Arunabh98 Jul 15, 2017

Author Contributor

This gets added on its own even after I remove it. Removed it for now.

This comment has been minimized.

Copy link
@seanlip

seanlip Jul 15, 2017

Member

Oh! If that's the case, don't remove it. It's probably needed. If you are missing an important index, production errors will occur.

GAE autogenerates index definitions when you make a query in the devserver that requires such an index.

This comment has been minimized.

Copy link
@seanlip

seanlip Jul 15, 2017

Member

(So, the other moral here is, make sure before you submit the PR that you've manually exercised all possible code paths by playing with the devserver)

This comment has been minimized.

Copy link
@Arunabh98

Arunabh98 Jul 15, 2017

Author Contributor

Yes, I'll be sure to test more extensively. Added them back for now.

This comment has been minimized.

Copy link
@seanlip

seanlip Jul 15, 2017

Member

Er -- my question still stands, though. What causes this index to be generated? We ought to know that.

If you're not sure about the code bit, what actions on the devserver cause this index to be generated?

This comment has been minimized.

Copy link
@Arunabh98

Arunabh98 Jul 16, 2017

Author Contributor

I tried everything but couldn't back this. I ran all tests too. I don't know really know what to do. :(

This comment has been minimized.

Copy link
@seanlip

seanlip Jul 17, 2017

Member

Sorry, you mean -- it doesn't automatically get added back when you poke around and test etc.? If so, that's fine, possibly it's just not even needed, and it's OK to leave deleted. (On the other hand, if you find that it auto-updates when you play around with the dev server / tests, then it's OK to leave it in.)

Giving LGTM on the assumption that it doesn't get auto-added and you don't actually need to leave it in, but if you do then please feel free to regenerate it and then submit.

Arunabh98 added some commits Jul 15, 2017

@@ -103,8 +103,18 @@ def get_multi(cls, entity_ids, include_deleted=False):
not found, or it has been deleted and include_deleted is False,
then the corresponding entry is None.
"""
entity_keys = [ndb.Key(cls, entity_id) for entity_id in entity_ids]
entity_keys = []

This comment has been minimized.

Copy link
@seanlip

seanlip Jul 15, 2017

Member

This needs comprehensive tests. In particular be careful to test the case of multiple things missing in irregular places, since the correctness is somewhat dependent on the order of insertions.

This comment has been minimized.

Copy link
@Arunabh98

Arunabh98 Jul 16, 2017

Author Contributor

Done!

@seanlip

This comment has been minimized.

Copy link
Member

commented Jul 15, 2017

Just two more points. Otherwise LGTM!

@seanlip

This comment has been minimized.

Copy link
Member

commented Jul 15, 2017

Hi @Arunabh98 -- one question. If it's not too much trouble, would you mind also updating get_most_recent_message() and get_message_count() in core/storage/feedback/gae_models.py to use the new message_count property you introduced? That would avoid a get_all() and filter() operation, and make the query much faster.

Thanks!

@Arunabh98

This comment has been minimized.

Copy link
Contributor Author

commented Jul 16, 2017

Modified the two functions!

@seanlip
Copy link
Member

left a comment

LGTM. Thanks, @Arunabh98!

@seanlip

This comment has been minimized.

Copy link
Member

commented Jul 17, 2017

Btw, the backend load test is failing. Any idea why? (Did we actually make anything more complicated in the most recent round of changes?)

@Arunabh98

This comment has been minimized.

Copy link
Contributor Author

commented Jul 18, 2017

Locally the backend test is running perfectly with speeds under 0.1s. But over here there seems to be some delay. Should I increase the limit to 1 second?

@seanlip

This comment has been minimized.

Copy link
Member

commented Jul 18, 2017

@Arunabh98

This comment has been minimized.

Copy link
Contributor Author

commented Jul 18, 2017

Changed the limit and printed the actual time.

@Arunabh98 Arunabh98 merged commit d6670e0 into oppia:develop Jul 18, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

giritheja added a commit to giritheja/oppia that referenced this pull request Jul 18, 2017

Merge remote-tracking branch 'upstream/develop' into next-job-controller
* upstream/develop:
  Learner dashboard project milestone 2.1 (oppia#3591)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.