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.

Sign up for GitHub

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

CLDR-16900 Shorten server response if unchanged; bug fix, refactor #3596

Merged
merged 1 commit into from
Apr 2, 2024
Merged

CLDR-16900 Shorten server response if unchanged; bug fix, refactor #3596

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 57 additions & 20 deletions tools/cldr-apps/js/src/esm/cldrAnnounce.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,11 @@ const schedule = new cldrSchedule.FetchSchedule(
let thePosts = null;

let callbackSetData = null;
let callbackSetCounts = null;
let callbackSetUnread = null;

let alreadyGotId = 0;

/**
* Get the number of unread announcements, to display in the main menu
*
Expand All @@ -36,29 +39,28 @@ async function getUnreadCount(setUnreadCount) {
if (setUnreadCount) {
callbackSetUnread = setUnreadCount;
}
await refresh(callbackSetData);
await refresh(callbackSetData, callbackSetCounts);
}

/**
* Refresh the Announcements page and/or unread count
*
* @param {Function} viewCallbackSetData the callback function for the Announcements page, or null
* @param {Function} viewCallbackSetCounts the callback function for the Announcements page, or null
*
* The callback function for setting the data may be null if the Announcements page isn't open and
* we're only getting the number of unread announcements to display in the main header
*/
async function refresh(viewCallbackSetData) {
async function refresh(viewCallbackSetData, viewCallbackSetCounts) {
if (DISABLE_ANNOUNCEMENTS) {
return;
}
if (viewCallbackSetData) {
if (!callbackSetData) {
// The Announcements page was just opened, so re-fetch the data immediately regardless
// of how recently it was fetched previously (for unread count)
schedule.reset();
Copy link
Member Author

Choose a reason for hiding this comment

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

This schedule.reset isn't needed here anymore since now AnnouncePanel.vue "created" method calls cldrAnnounce.resetSchedule

}
callbackSetData = viewCallbackSetData;
}
if (viewCallbackSetCounts) {
callbackSetCounts = viewCallbackSetCounts;
}
if (!cldrStatus.getSurveyUser()) {
if (viewCallbackSetData) {
viewCallbackSetData(null);
Expand All @@ -68,36 +70,64 @@ async function refresh(viewCallbackSetData) {
if (schedule.tooSoon()) {
return;
}
const url = cldrAjax.makeApiUrl("announce", null);
let p = null;
if (alreadyGotId) {
p = new URLSearchParams();
p.append("alreadyGotId", alreadyGotId);
}
const url = cldrAjax.makeApiUrl("announce", p);
schedule.setRequestTime();
return await cldrAjax
.doFetch(url)
.then(cldrAjax.handleFetchErrors)
.then((r) => r.json())
.then(setPosts)
.catch((e) => console.error(e));
.catch(handleErrorOrNotModified);
}

function handleErrorOrNotModified(e) {
if (e.message === "Not Modified") {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (e.message === "Not Modified") {
if (e.status === 304) {

should be able to check the status code directly - the message can be variable.

Copy link
Member Author

Choose a reason for hiding this comment

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

That would require changes to cldrAjax -- currently cldrAjax.handleFetchErrors is as follows:

function handleFetchErrors(response) {
  if (!response.ok) {
    throw new Error(response.statusText);
  }
  return response;
}

The Error only contains response.statusText -- I'm not sure how to set e.status in a standard way

// 304
if (CLDR_ANNOUNCE_DEBUG) {
console.log("cldrAnnounce got " + e.message);
}
} else {
console.error(e);
}
}

function setPosts(json) {
thePosts = json;
schedule.setResponseTime();
setAlreadyGotId(thePosts);
const totalCount = thePosts.announcements?.length || 0;
let checkedCount = 0;
for (let announcement of thePosts.announcements) {
if (announcement.checked) {
++checkedCount;
}
}
const unreadCount = totalCount - checkedCount;
if (callbackSetData) {
callbackSetData(thePosts);
callbackSetData(thePosts); // AnnouncePanel.vue
}
if (callbackSetCounts) {
callbackSetCounts(unreadCount, totalCount); // AnnouncePanel.vue
}
if (callbackSetUnread) {
const totalCount = thePosts.announcements?.length || 0;
let checkedCount = 0;
for (let announcement of thePosts.announcements) {
if (announcement.checked) {
++checkedCount;
}
}
const unreadCount = totalCount - checkedCount;
callbackSetUnread(unreadCount);
callbackSetUnread(unreadCount); // MainHeader.vue (balloon icon)
}
return thePosts;
}

function setAlreadyGotId(thePosts) {
for (let announcement of thePosts.announcements) {
if (announcement.id > alreadyGotId) {
alreadyGotId = announcement.id;
}
}
}

function canAnnounce() {
return cldrStatus.getPermissions()?.userIsManager || false;
}
Expand All @@ -107,7 +137,7 @@ function canChooseAllOrgs() {
}

async function compose(formState, viewCallbackComposeResult) {
schedule.reset();
resetSchedule();
const init = cldrAjax.makePostData(formState);
const url = cldrAjax.makeApiUrl("announce", null);
return await cldrAjax
Expand All @@ -119,6 +149,7 @@ async function compose(formState, viewCallbackComposeResult) {
}

async function saveCheckmark(checked, announcement) {
resetSchedule();
Copy link
Member Author

Choose a reason for hiding this comment

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

this fixes a bug -- otherwise, the "unread" number by the balloon icon in the main menu took 60 seconds before it updated in response to adding a checkmark for "I have read this"

window.setTimeout(refreshCount, 1000);
const init = cldrAjax.makePostData({
id: announcement.id,
Expand Down Expand Up @@ -154,12 +185,18 @@ async function combineAndValidateLocales(locs, validateLocCallback) {
.catch((e) => console.error(`Error: ${e} validating locales`));
}

function resetSchedule() {
alreadyGotId = 0;
schedule.reset();
}

export {
canAnnounce,
canChooseAllOrgs,
compose,
getUnreadCount,
refresh,
resetSchedule,
saveCheckmark,
combineAndValidateLocales,
};
23 changes: 8 additions & 15 deletions tools/cldr-apps/js/src/views/AnnouncePanel.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,8 @@ export default {
created() {
this.canAnnounce = cldrAnnounce.canAnnounce();
this.canChooseAllOrgs = cldrAnnounce.canChooseAllOrgs();
cldrAnnounce.refresh(this.setData);
cldrAnnounce.resetSchedule();
cldrAnnounce.refresh(this.setData, this.setCounts);
},

methods: {
Expand All @@ -92,24 +93,16 @@ export default {
this.pleaseLogIn = true;
} else {
this.announcementData = data;
this.updateCounts();
}
},

checkmarkChanged(event, announcement) {
cldrAnnounce.saveCheckmark(event.target.checked, announcement);
this.updateCounts();
setCounts(unreadCount, totalCount) {
this.unreadCount = unreadCount;
this.totalCount = totalCount;
},

updateCounts() {
this.totalCount = this.announcementData.announcements.length;
let checkedCount = 0;
for (let announcement of this.announcementData.announcements) {
if (announcement.checked) {
++checkedCount;
}
}
this.unreadCount = this.totalCount - checkedCount;
checkmarkChanged(event, announcement) {
cldrAnnounce.saveCheckmark(event.target.checked, announcement);
},

startCompose() {
Expand Down Expand Up @@ -137,7 +130,7 @@ export default {
message: "Your announcement was not posted: " + errMessage,
});
}
cldrAnnounce.refresh(this.setData);
cldrAnnounce.refresh(this.setData, this.setCounts);
},
},
};
Expand Down
10 changes: 7 additions & 3 deletions tools/cldr-apps/js/test/Test.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,13 @@ <h1>CLDR SurveyTool JavaScript Tests</h1>
<ul>
<li>in the 'js' directory, run <code>npm i</code> to install</li>
<li>To rebuild, run <code>npm run build-test</code></li>
<li>If you want to continuously compile, keep <code>npm run watch-test</code> (control-C to cancel)</li>
<li>open the browser console to check
for errors/warnings that are otherwise invisible!
<li>
If you want to continuously compile, keep
<code>npm run watch-test</code> (control-C to cancel)
</li>
<li>
open the browser console to check for errors/warnings that are
otherwise invisible!
</li>
</ul>
<p><em>References:</em></p>
Expand Down
17 changes: 13 additions & 4 deletions tools/cldr-apps/src/main/java/org/unicode/cldr/web/AnnouncementData.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,12 @@ public class AnnouncementData {

static boolean dbIsSetUp = false;

static int mostRecentAnnouncementId = 0;

public static int getMostRecentAnnouncementId() {
return mostRecentAnnouncementId;
}

public static void get(
UserRegistry.User user, List<Announcements.Announcement> announcementList) {
makeSureDbSetup();
Expand All @@ -30,6 +36,9 @@ public static void get(
if (aFilter.passes(a)) {
a.setChecked(getChecked(a.id, user.id));
announcementList.add(a);
if (a.id > mostRecentAnnouncementId) {
mostRecentAnnouncementId = a.id;
}
}
}
} finally {
Expand All @@ -55,7 +64,6 @@ private static Announcements.Announcement makeAnnouncementFromDbObject(Object[]
String audience = (String) objects[7];

String date = lastDate.toString();
// long date_long = lastDate.getTime();
Announcements.Announcement a =
new Announcements.Announcement(id, poster, date, subject, body);
a.setFilters(locs, orgs, audience);
Expand All @@ -69,10 +77,11 @@ public static void submit(
throws SurveyException {
makeSureDbSetup();
try {
int announcementId = savePostToDb(request, user);
final int announcementId = savePostToDb(request, user);
logger.fine("Saved announcement, id = " + announcementId);
response.id = announcementId;
response.ok = true;
mostRecentAnnouncementId = announcementId;
} catch (SurveyException e) {
response.err = "An exception occured: " + e;
logger.severe(e.getMessage());
Expand Down Expand Up @@ -117,7 +126,7 @@ private static PreparedStatement prepare_pAdd(Connection conn) throws SQLExcepti
"INSERT INTO "
+ DBUtils.Table.ANNOUNCE
+ " (poster,subj,text,locs,orgs,audience)"
+ " values (?,?,?,?,?,?)";
+ " VALUES (?,?,?,?,?,?)";
return DBUtils.prepareStatement(conn, "pAddAnnouncement", sql);
}

Expand All @@ -134,7 +143,7 @@ private static void emailNotify(
+ announcementId
+ " queueing:"
+ recipients.size());
if (recipients.size() == 0) {
if (recipients.isEmpty()) {
return;
}
String subject = "CLDR Survey Tool announcement: " + request.subject;
Expand Down
10 changes: 9 additions & 1 deletion tools/cldr-apps/src/main/java/org/unicode/cldr/web/api/Announcements.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,12 @@ public class Announcements {
mediaType = "application/json",
schema = @Schema(implementation = STError.class))),
})
public Response getAnnouncements(@HeaderParam(Auth.SESSION_HEADER) String sessionString) {
public Response getAnnouncements(
@HeaderParam(Auth.SESSION_HEADER) String sessionString,
@QueryParam("alreadyGotId")
@Schema(description = "The client already got this announcement ID")
@DefaultValue("0")
int alreadyGotId) {
CookieSession session = Auth.getSession(sessionString);
if (session == null) {
return Auth.noSessionResponse();
Expand All @@ -71,6 +76,9 @@ public Response getAnnouncements(@HeaderParam(Auth.SESSION_HEADER) String sessio
if (SurveyMain.isBusted() || !SurveyMain.wasInitCalled() || !SurveyMain.triedToStartUp()) {
return STError.surveyNotQuiteReady();
}
if (alreadyGotId != 0 && alreadyGotId == AnnouncementData.getMostRecentAnnouncementId()) {
return Response.notModified().build();
Copy link
Member Author

Choose a reason for hiding this comment

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

I have some doubt whether this is a standard way to use notModified (304)...

Possible alternatives include (1) setting headers so that the browser delivers cached response for 304 (but that's maybe less efficient -- needless work processing the identical json); (2) return 200 and short json, e.g., just json.unchanged = true; (3) Etag...

}
Comment on lines +79 to +81
Copy link
Member

@srl295 srl295 Apr 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

might consider adding this here (see entry for 503, etc in the annotations on this function)

    @APIResponse(responseCode = "304", description = "No further announcements (past alreadyGotId"),

Copy link
Member Author

Choose a reason for hiding this comment

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

good idea

if tests pass, maybe best to merge this as-is since it does include bug fix and performance improvement; but keep ticket open and consider protocol improvements under discussion

AnnouncementResponse response = new AnnouncementResponse(session.user);
return Response.ok(response).build();
}
Expand Down
Loading