uzh / marugoto

Marugoto: eLearning Framework for Story Telling
GNU General Public License v2.0
8 stars 6 forks source link

Two mails in Inbox instead of one (2.2.1) #28

Closed martindusinberre closed 4 years ago

martindusinberre commented 5 years ago

On 2.2.1, mail1.json generates two emails in the Inbox. It should only be one.

christian-bernet commented 5 years ago

Estimate 2 hours

christian-bernet commented 5 years ago

we can not reproduce this situation. Maybe there was an error in the content that is now away. @martindusinberre can you please verify.

martindusinberre commented 5 years ago

Hi Christian: please reassign this to me once #51 has been fixed, and then I can test for the two-email problem on 2.2.1 again.

christian-bernet commented 5 years ago

ok. Assigned now to you. Please test now again the email problem on 2.2.1.

martindusinberre commented 5 years ago

Sorry, the bug is still there.

Screen Shot 2019-09-19 at 09 46 53
christian-bernet commented 5 years ago

This situation is because of corrupt content after importing it with the importer. It is not because of wrong .json files. These are ok. So this is also a result of the unstable importer.

Because this is wasting time for searching each time on such corrupt content situations (we had this situation already two or three times), we decided to change the importer to a simple concept like this:

christian-bernet commented 5 years ago

that is still open. If you do a second import it still creates two mails! This is an importer bug. I will check that after my vacation.

christian-bernet commented 4 years ago

can not reproduce it locally any more at this time. But importer needs still a bit of code changes.

christian-bernet commented 4 years ago

Title of page 2.2.1 is "Chapter 2/5: Back home" (ask prof. for more time, to get there) Bug is still there.

christian-bernet commented 4 years ago

This bug ist still here: image

interrogator commented 4 years ago

@christian-bernet if and when this gets fixed, please let me know so I can version bump and redeploy livesintransit.org...

christian-bernet commented 4 years ago

@interrogator I will fix this. But it is not that easy... I have looked several times at this issue. And I will also do that on the beginning next week. But I am not sure if its fixed afterwards. We have the problem only at the test-system. Not local on our dev-systems. So it is a little bit tricky to fix. But I will let you keep you up to date.

christian-bernet commented 4 years ago

@interrogator it is planned to look at this next week. I will keep you up to date for that.

christian-bernet commented 4 years ago

We postponed to do this.... It is now planned at the 19.12.19

martindusinberre commented 4 years ago

OK, thanks for the update. Please do make sure that it's fixed no later than the 19th, as Pim's last day is on the 20th and we would all like to have the outstanding bugs from August fixed by then.

christian-bernet commented 4 years ago

Thanks. For the feedback. So we will do it not later than the 19th.

interrogator commented 4 years ago

@christian-bernet I don't know the backend well, but in terms of simplest fix, since mails are apparently exact duplicates, why not simply implement some check like (in python for brevity):

set_of_mails = set()
if send_triggered(mail):
    if mail not in set_of_mails:
        send(mail)
        set_of_mails.add(mail)
    else:
        log("send mail triggered for duplicate msg', mail, debug_info')

Even if we don't know exactly why the problem occurs, at least this would stop it and collect info that might help us diagnose at a later stage. just a suggestion if you can't reproduce the problem...

christian-bernet commented 4 years ago

@interrogator Great idea..... thanks.

But happily: We test it again right now at the stage system and the problem is now also gone. I assume that the problem was because of this instable importer. But with the new version that problem is gone. So, please retest it on your side and if everything is also ok, then please close it.

martindusinberre commented 4 years ago

Hi Christian: great, thanks for the update. @lfeine could you please test this when you have time? Best, Martin

interrogator commented 4 years ago

@christian-bernet are the changes related to this issue all in master branches? If so, this issue can be closed, as changes will go into next deployment.

christian-bernet commented 4 years ago

@interrogator We did not change anything this week. So, yes. this changes are all in master branch.

lfeine commented 4 years ago

I'm not sure the previous messages mean that this still needs testing, but it doesn't show up to me anymore.

Unfortunately, when replying to the formerly duplicated E-Mail, the reply button appears to not do anything (no state changes, no loading bar). The mail is still sent however, and appears when swapping back from another mail or closing and opening the mail window.

The error that appears in the console after pressing the reply button is: Error: Request failed with status code 400 app.js line 628 > eval:16:15 (Firefox 71.0)

interrogator commented 4 years ago

@christian-bernet

christian-bernet commented 4 years ago

I had a quick look on it. image

There is an auto page transition when you reply to that mail which will fail, if you are not on page 2.2.1. I can fix that on Monday. Unfortunately I can't do it today because we reserved the day for you last Monday.

@martindusinberre Does that work for you?

martindusinberre commented 4 years ago

@christian-bernet I had a look at this and I think it's a problem in the backend, right, not in the JSON files that I have. If so, then yes, please fix. The idea is that you shouldn't be able to transition from 2.2.1 to 2.2.2 unless you answer the email 2.2.1/mail1.json Thanks, Martin

christian-bernet commented 4 years ago

Bug for mail reply fixed: d3ff4ad8 @martindusinberre but you have to change the jsons a litte bit, if the page transition from 2.2.1 to 2.2.2 should only be available, if you reply to the mail on 2.2.1. You have to remove the pageTransition in mail1.json on page 2.2.1 and it will work. With the pageTransition in mail1.json an automatic pageTransition after you reply to the mail will happens. But to have that on the reply of the same mail is not possible at the moment. Technically this gives a circle relationship, which the system at the moment can not handle.

Please let me know, if it is now ok for you like this. All best, Christian

christian-bernet commented 4 years ago

We will have again a look at this tomorrow and see if we can fix the problem with the circle relationship for pagetransition on page 2.2.1

martindusinberre commented 4 years ago

Many thanks, Christian. I'm not sure why there's a particular problem here, as it should be a similar functionality as on 2.2.8, where you can only transition to 2.2.9 if you answer the previously received email (2.1.6). Here's it's always functioned fine, so I'm a bit surprised if the transition from 2.2.1 to 2.2.2, dependent on answering 2.2.1/mail1, doesn't work. Or have I misunderstood? Anyway, let me know if it's faster to talk by phone about this tomorrow--I'm around all day after about 9am.

nemtish commented 4 years ago

Hi @martindusinberre for this kind of situations where user should be transitioned automatically when replies to mail, you have to change "pageTransition.json" file like below: "affectedMail": null, "buttonText": null

With this change button will stay invisible for the user and circle relationship (@christian-bernet mentioned) won't be problem

Pages where you have page transitions that are only unlocked (without auto-transition) when user reply to specific mail you can leave like it is

christian-bernet commented 4 years ago

Unfortunately we can not change the content ourself, because we do not have access to the content git anymore. Add for the page 2.2.8 there is no auto-transition (if user reply to the mail) active. Auto-transition can be activated if you add the pagetranstion in the mail. As it is for mail1 on page 2.2.1. image